New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(platform-server): fix get/set title for parse5 adapter #14965

Merged

chuckjaz merged 1 commit into angular:master from FrozenPandaz:title

Mar 13, 2017

Contributor

FrozenPandaz commented Mar 6, 2017

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (check one with "x")

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)

Fixes #14936

Currently, setting and getting the title on platform-server is broken and untested.

What is the new behavior?

Getting and setting the title will function and be tested

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[x] No

googlebot added the cla: yes label

Contributor Author

FrozenPandaz commented Mar 6, 2017

I'm planning a change which will cache the document's title the first time its retrieved so we dont have to query every time.

Contributor

DzmitryShylovich commented Mar 6, 2017

They are moving away from parse5 #14939

Contributor Author

FrozenPandaz commented Mar 6, 2017

@DzmitryShylovich It's an on going discussion

Contributor

DzmitryShylovich commented Mar 6, 2017

41 file have been changed. doesn't look like a discussion :)

Contributor

vikerman commented Mar 6, 2017

Hey @DzmitryShylovich - I've been in sync with @FrozenPandaz and asked him to submit this PR. The PR to move to jsdom is still experimental. We still have to figure out whether it is performant enough. Thanks.

vikerman self-requested a review

March 6, 2017 23:02

vikerman added the action: review label

vikerman suggested changes

View reviewed changes

Contributor

vikerman left a comment

Please fix the commit message to fix(platform-server):...

modules/@angular/platform-server/src/parse5_adapter.ts Outdated

@@ @@ -592,6 +596,16 @@ export class Parse5DomAdapter extends DomAdapter { @@
                 getCookie(name: string): string { throw new Error('not implemented'); }
                 setCookie(name: string, value: string) { throw new Error('not implemented'); }
                 animate(element: any, keyframes: any[], options: any): any { throw new Error('not implemented'); }
+                private getTitleNode(doc: any) {
+                  let title = this.querySelector(doc, 'head title');

Contributor

vikerman Mar 6, 2017

Can you just query doc.head for title?

modules/@angular/platform-server/src/parse5_adapter.ts Outdated

+                  }
+                  title = this.createElement('title');
+                  this.appendChild(this.querySelector(doc, 'head'), title);

Contributor

vikerman Mar 6, 2017

doc.head should already be set to the head element

Contributor Author

FrozenPandaz Mar 6, 2017

I thought so too.. but i was having trouble.. i will double check

modules/@angular/platform-server/test/integration_spec.ts Outdated

+                        getDOM().setTitle(doc, 'Test');
+                        const title = getDOM().querySelector(doc, 'head title');
+                        expect(title).toBeTruthy();
+                        expect(getDOM().getText(title)).toContain('Test');

Contributor

vikerman Mar 6, 2017

Can you make it stricter to .toBe('Test') ?

modules/@angular/platform-server/test/integration_spec.ts Outdated

-                     }));
+                    }));
+                  it('adds title to the document', async(() => {

Contributor

vikerman Mar 6, 2017

Thanks for adding this integration test!

Contributor Author

FrozenPandaz Mar 6, 2017

Then it'll never happen again :)

modules/@angular/platform-server/test/integration_spec.ts Outdated

+                        const title = getDOM().querySelector(doc, 'head title');
+                        expect(title).toBeTruthy();
+                        expect(getDOM().getText(title)).toContain('Test');
+                      });

Contributor

vikerman Mar 6, 2017

For a more end to end check we should do a PlatformState.renderToString() and make sure the title is rendered

Contributor Author

FrozenPandaz Mar 6, 2017

Good idea, i think i'll make it a separate it() so we know the difference

vikerman suggested changes

View reviewed changes

modules/@angular/platform-server/test/integration_spec.ts Outdated

+                      }]);
+                      platform.bootstrapModule(ExampleModule).then(ref => {
+                        const doc = ref.injector.get(DOCUMENT);
+                        getDOM().setTitle(doc, 'Test');

Contributor

vikerman Mar 7, 2017

Please use the Title service in the test. That's how we want end users to set the title.

FrozenPandaz changed the title ~~fix(@angular/platform-server): fix get/set title for parse5 adapter~~ fix(platform-server): fix get/set title for parse5 adapter

FrozenPandaz force-pushed the title branch 2 times, most recently from 3b620cd to 693d871 Compare

March 7, 2017 03:22

vikerman approved these changes

View reviewed changes

modules/@angular/platform-server/test/integration_spec.ts Outdated

@@ @@ -145,6 +155,33 @@ export function main() { @@
                        });
                      }));
+                  it('adds title to the document', async(() => {

Contributor

vikerman Mar 7, 2017

Can you remove this test? We just want to test public API in the integration test.

vikerman added action: cleanup and removed action: review labels

FrozenPandaz force-pushed the title branch 4 times, most recently from 9dc7d88 to 6267578 Compare

March 7, 2017 21:03

googlebot commented Mar 7, 2017

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

googlebot added cla: no and removed cla: yes labels

FrozenPandaz force-pushed the title branch from 6267578 to 331b48a Compare

March 7, 2017 21:07

googlebot commented Mar 7, 2017

CLAs look good, thanks!

googlebot added cla: yes and removed cla: no labels

FrozenPandaz closed this

FrozenPandaz force-pushed the title branch from 331b48a to 1cff125 Compare

March 7, 2017 21:23

FrozenPandaz reopened this

vikerman suggested changes

View reviewed changes

modules/@angular/platform-server/src/parse5_adapter.ts Outdated

@@ @@ -30,7 +30,7 @@ function _notImplemented(methodName: string) { @@
               /**
                * Parses a document string to a Document object.
                */
-              export function parseDocument(html: string) {
+              export function parseDocument(html: string): Document {

Contributor

vikerman Mar 7, 2017

Please leave the return type as any - since it's a parse5 Document and not the standard TS Document interface,

modules/@angular/platform-server/src/parse5_adapter.ts Outdated

@@ @@ -43,6 +43,7 @@ export function parseDocument(html: string) { @@
                * can introduce XSS risks.
                */
               export class Parse5DomAdapter extends DomAdapter {
+                private title: HTMLTitleElement;

Contributor

vikerman Mar 7, 2017

Please leave the type of this as any - This is a parse5 Element and not an actual HTMLTitleElement.

FrozenPandaz force-pushed the title branch from d30d1e9 to 2100f6a Compare

March 7, 2017 21:36

vikerman approved these changes

View reviewed changes

vikerman added action: merge and removed action: cleanup labels

Contributor

chuckjaz commented Mar 10, 2017

This PR needs to be rebased.

FrozenPandaz force-pushed the title branch from 2100f6a to 68a780f Compare

March 10, 2017 23:01

Contributor Author

FrozenPandaz commented Mar 10, 2017

rebased


          fix(platform-server): fix get/set title in parse5 adapter

ef68bf5

FrozenPandaz force-pushed the title branch from 68a780f to ef68bf5 Compare

March 11, 2017 19:58

vikerman approved these changes

View reviewed changes

chuckjaz added the pr_action: queued label

chuckjaz merged commit 018e5c9 into angular:master

FrozenPandaz deleted the title branch

March 14, 2017 22:40

SamVerschueren pushed a commit to SamVerschueren/angular that referenced this pull request


          fix(platform-server): fix get/set title in parse5 adapter (angular#14965

2ad3645

asnowwolf pushed a commit to asnowwolf/angular that referenced this pull request


          fix(platform-server): fix get/set title in parse5 adapter (angular#14965

4dae001

juleskremer pushed a commit to juleskremer/angular that referenced this pull request


          fix(platform-server): fix get/set title in parse5 adapter (angular#14965

20a9e3b

angular-automatic-lock-bot bot commented Sep 11, 2019

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

angular-automatic-lock-bot bot locked and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge cla: yes