feat(platform browser): introduce Meta service #12322

DzmitryShylovich · 2016-10-15T09:18:02Z

DzmitryShylovich · 2016-10-15T12:28:46Z

CI fails with

+/tmp/angular-build/typings-test.1476534211/node_modules/.bin/tsc -p tsconfig.json
node_modules/@angular/platform-browser/src/browser/meta.d.ts(13,46): error TS1110: Type expected.
node_modules/@angular/platform-browser/src/browser/meta.d.ts(26,44): error TS1110: Type expected.

I don't know what I'm missing.

jeffbcross

General question, is there any benefit to setting meta tags in the browser (instead of just pre-render)? Like do Chrome extensions like Pocket look at the current meta tags of content?

jeffbcross · 2016-10-24T19:47:16Z

modules/@angular/platform-browser/src/browser/meta.ts

+   * @param contentValue new content value
+   * @returns {void}
+   */
+  setContent(metaName: string, contentValue: string): void {


What is the reasoning for not implicitly creating the tag if it doesn't exist? For example, for something like open graph tags, the existence of certain types of tags depends on the page's content. I.e. the og:video tag should not be present if there's no video.

Some meta tags have a "property" and no "name" such as Facebook:

Facebook Open Graph

I know this is not web standard but is this something that should be implemented in like a PropertyMetaTag or something?

W3 Meta Tag

Agreed. Almost any respectable application will use property tags as well, even if they are not standard.
If this doesn't get implemented natively, then we'll have to do our own service anyway, which is just duplicating code/effort.

@FrozenPandaz I think you're agreeing with my original comment? My comment was positing that the existence of some tags, such as open graph tags, depends on content, so the meta service should manage creating/removing tags instead of just updating attributes of existing tags.

One of my other review comments addresses changing the API to support arbitrary attribute names.

I was just commenting on the same code..

What I meant is that Metas should not only have a name property but also a... property property.

I do agree with your comment though. There is no one set of metadata for every single app. I believe we should start with a blank slate and have the developer set them 1 by 1.

It should create it if its not there already, edit it if it is there, and it should have the ability to remove it.. I wonder how that will work with SPAs.. we might want to have a removeAll method as well.

jeffbcross · 2016-10-24T19:56:49Z

modules/@angular/platform-browser/src/browser/meta.ts

+   * @param metaName meta tag name
+   * @returns {HTMLElement|null}
+   */
+  getMeta(metaName: string): HTMLElement|null { return getDOM().query(`meta[name=${metaName}]`); }


I'd rather the get/set methods be more generic than include the getMeta method and return the element. setContent could accept an optional third argument attributeName='content'.

Or is there another reason to have the getMeta method other than setting arbitrary attributes?

jeffbcross · 2016-10-24T19:57:05Z

modules/@angular/platform-browser/src/browser/meta.ts

+  getContent(metaName: string): string|null {
+    const meta: HTMLElement = this.getMeta(metaName);
+    return meta ? meta.getAttribute(CONTENT_ATTR) : null;
+  }


A remove method seems appropriate since this service is a map-like interface, as long as the setContent method would implicitly create elements.

jeffbcross · 2016-10-24T20:30:07Z

By the way the TS failures are because of returning union types, which is not compatible with TS 1.8.

lacolaco · 2016-10-25T06:31:56Z

@jeffbcross It's needed to change metadata for sharing content after each routing navigation. This is for second+ views.

alfaproject · 2016-10-25T17:21:13Z

modules/@angular/platform-browser/test/browser/meta_spec.ts

+      expect(actual).toEqual(META_CONTENT);
+    });
+
+    it('should return meta element', () => {


Repeated description

zoechi · 2016-10-30T17:09:03Z

Does this work with server-side rendering and Web Worker when it reads from the DOM?

jeffbcross · 2016-10-31T21:30:14Z

@laco0416 thanks, I'm looking for documentation about common extensions like pocket to verify that they actually read the meta tags inside the browser, rather than re-requesting the content. Can you provide any links to confirm that popular extensions actually look at the browser state?

jeffbcross · 2016-10-31T21:31:36Z

@DzmitryShylovich do you plan to update this PR with requested changes?

kodeine · 2016-11-05T23:28:35Z

@DzmitryShylovich @jeffbcross
any idea when this will be fixed and merged? :)

DzmitryShylovich · 2016-11-06T17:11:04Z

@jeffbcross updated

kodeine · 2016-11-08T19:22:19Z

@DzmitryShylovich can you please check, some of the tests didnt pass?

vicb · 2016-11-10T22:48:46Z

modules/@angular/platform-browser/src/browser/meta.ts

+   * @param tags
+   * @returns {HTMLMetaElement[]}
+   */
+  addTag(...tags: MetaDefinition[]): HTMLMetaElement[] {


could we make this addTags and pass an Array always ?

(In the currrent impl isArray is useless)

vicb · 2016-11-10T22:49:37Z

modules/@angular/platform-browser/src/browser/meta.ts

+   * @returns {HTMLMetaElement}
+   */
+  getTag(selector: string): HTMLMetaElement {
+    if (selector == null || selector === '') return null;


if (!selector)

vicb · 2016-11-10T22:50:13Z

modules/@angular/platform-browser/src/browser/meta.ts

+   * @returns {HTMLMetaElement}
+   */
+  updateTag(selector: string, tag: MetaDefinition): HTMLMetaElement {
+    let meta: HTMLMetaElement = this.getTag(selector);


vicb · 2016-11-10T22:50:42Z

modules/@angular/platform-browser/src/browser/meta.ts

+   *
+   * @param selectorOrElem selector or meta element
+   */
+  removeTag(selectorOrElem: string|HTMLMetaElement): void {


removeTagBySelector & removeTagElement

DzmitryShylovich · 2016-11-11T10:51:26Z

updated

vicb · 2016-11-11T17:09:17Z

modules/@angular/platform-browser/src/browser/meta.ts

+   * @param tags
+   * @returns {HTMLMetaElement[]}
+   */
+  addTags(...tags: Array<MetaDefinition|MetaDefinition[]>): HTMLMetaElement[] {


why do we need [][] here ? (over addTags(tags: MD[]))

Just a convenience so we can add meta tags like

addTags(tag1, tag2, tag3)

or

addTags([tag1, tag2, tag3])

Otherwise we will get such request #7432 :)

Make it addTag and nobody can argue !

vicb · 2016-11-11T17:11:15Z

modules/@angular/platform-browser/src/browser/meta.ts

+   */
+  removeTagElement(meta: HTMLMetaElement): void {
+    if (meta) {
+      this._removeMetaElement(meta);


what about inlining methods when called once ony ?

vicb · 2016-11-11T17:13:24Z

modules/@angular/platform-browser/src/browser/meta.ts

+   */
+  getTag(selector: string): HTMLMetaElement {
+    if (!selector) return null;
+    return this._dom.query(`meta[${selector}]`);


what if multiple matching els ?

Do u think we should use DomAdapter.querySelectorAll here?

I would tend to say probably (then updateTag makes little sense)

So what should we do? I think update is the most wanted method here.

often useful to change meta values as well.

from original issue

For example in vuejs they use id to uniquely identify a tag.

updateAll ?

is this really a problem? http://stackoverflow.com/a/26623223
Looks like it's not possible to have multiple meta tags with the same name/property.

~~btw not sure how it will work if we dynamically will add tags with the same name...~~
Just tested in chrome. we can add the same tag over and over again and then query them all as an array :)

What does the spec says ?
Anyway there should be tests (executed BrowserStack and SauceLabs)

Spec doesn't say anything about multiple tags with the same name https://www.w3.org/TR/html5/document-metadata.html#the-meta-element

vicb · 2016-11-11T18:37:44Z

modules/@angular/platform-browser/src/browser/meta.ts

+ *
+ * ### Example
+ *
+ * ```ts


I'm very much a bug fan of inline docs but also I think too much docs is bad, can you reduce the # of exs here ?

DzmitryShylovich · 2016-12-05T23:47:58Z

I'll add new commits so it will be easier to review. Will squash them later.

vicb · 2016-12-06T22:02:15Z

modules/@angular/platform-browser/src/browser/meta.ts

+        result.push(list[i]);
+      }
+    }
+    return result;


return list ? [].slice.call(list) : [];

vicb · 2016-12-06T22:04:50Z

modules/@angular/platform-browser/test/browser/meta_spec.ts

+      getDOM().getElementsByTagName(doc, 'head')[0].appendChild(defaultMeta);
+    });
+
+    afterEach(() => getDOM().getElementsByTagName(doc, 'head')[0].removeChild(defaultMeta));


clean up between tests so they won't affect each other.

Currently I use native document to create elements so I need to clean up them between test. Probably I should use fakeDoc = getDOM().createHtmlDocument(); instead

Sorry I meant remove over rempveChild - cleaning up is great !

vicb · 2016-12-06T22:08:53Z

modules/@angular/platform-browser/src/browser/meta.ts

+    return el;
+  }
+
+  private tryParseSelector(tag: MetaDefinition): string {


why try here ?

Because we are trying our best to create a selector from a tag.
For example, a tag can be {content: 'my content'} (without name or property). In this case we cannot construct selector and I think we should throw an error.

try (the word) makes me think try (the keyword)

parseSelector() returning null would be ok
parseSelectorOrThrow() throwing would also be ok

vicb · 2016-12-06T22:09:25Z

modules/@angular/platform-browser/src/browser/meta.ts

+ */
+@Injectable()
+export class Meta {
+  private _dom: DomAdapter = getDOM();


inject the DOM ? (factory)

is it possible to inject dom adapter?

you can inject whatever when using a factory fn to define the binding. Makes sense ?

do u want me to create a factory provider {provide: DomAdapter: useFactory: getDOM}? :)

DzmitryShylovich · 2016-12-07T01:15:53Z

ie doesn't support remove https://travis-ci.org/angular/angular/jobs/181829865 :)

vicb · 2016-12-07T02:16:48Z

BrowserDomAdapter.remove() uses removeChild

vicb · 2016-12-07T02:17:34Z

modules/@angular/platform-browser/test/browser/meta_spec.ts

+      getDOM().getElementsByTagName(doc, 'head')[0].appendChild(defaultMeta);
+    });
+
+    afterEach(() => defaultMeta.remove());


getDom().remove(defaultMeta);

... or use your own service :)

DzmitryShylovich · 2016-12-07T23:58:43Z

@vicb so what about dom adapter injection? I don't fully understand your point.

vicb · 2016-12-08T00:02:35Z

// ...
{provide: Meta, useFactory: getMeta}
// ...

function getMeta(): Meta {
  return new Meta(getDom());
}

DzmitryShylovich · 2016-12-08T12:17:36Z

updated (seems circleci is dead)

vicb · 2016-12-09T02:44:07Z

Thanks !

MarkPieszak · 2016-12-12T01:59:52Z

Do we know if this is slated for 2.4? @vicb

jeffbcross · 2016-12-12T17:41:47Z

@MarkPieszak I believe there is no 2.4 release planned (see 2.3 blog post) so this would be in the first beta for 4.0 (See @IgorMinar's talk on the subject).

MarkPieszak · 2016-12-12T19:18:38Z

Family over past few days, I saw something about that, but thought it was an April Fools joke (in December) haha.

So March then, great! I was just wondering since this makes life easier for Universal users, think we talked about that the other week.

jeffbcross · 2016-12-12T22:43:43Z

@MarkPieszak yep, a big motivator of this API was for pre-rendering. Final will be in March, but betas will start being released soon :).

MarkPieszak · 2016-12-12T22:54:15Z

@jeffbcross great! Messaged you on the slack, not sure if you still use that one

kodeine · 2017-01-25T06:30:19Z

@DzmitryShylovich is there any docs on how to use this? Also which version of angular this is supported?

DzmitryShylovich · 2017-01-25T06:53:10Z

@kodeine

is there any docs on how to use this?

not yet. you can find usage examples in the tests

which version of angular this is supported?

beta 4.0

goelinsights · 2017-04-07T02:04:49Z

@DzmitryShylovich @jeffbcross was looking at the new API for meta -- is it possible to use it for link rel= tags as well?

Or should we stick with the old pattern along the lines below. If the second, do you know what where the old getDOM moved to?...this import broke for me

import { getDOM } from '@angular/platform-browser/src/dom/dom_adapter';

// ...
this.DOM = getDOM();

private getOrCreateLinkElement(name: string): HTMLElement {
  let el: HTMLElement;
  el = this.DOM.query('link[rel=' + name + ']');
  if (el === null) {
    el = this.DOM.createElement('link');
    el.setAttribute('rel', name);
    this.headElement.appendChild(el);
  }
  return el;
}

MarkPieszak · 2017-04-07T05:03:55Z

Check out here for a temporary solution until a DocumentService is implemented within Core
#15776 (comment)

fulls1z3 · 2017-04-10T09:06:34Z

@MarkPieszak I checked the linkService and it seems awesome.

Actualy I am planning to use such a similar htmlService to set the lang attribute of html tag (to the working language) as well as canonical links, right at the meta utility (@nglibs/meta) generating the meta tags based on route definitions.

Meanwhile, many thanks to @DzmitryShylovich for Meta/Title services. This way, the meta utility avoids direct manipulation of DOM and just depends on DomAdapter (in Angular 2) or on Meta/Title services (in Angular 4) - and works well with server-side rendering as well as client-side rendering.

angular-automatic-lock-bot · 2019-09-11T06:21:44Z

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.}

googlebot added the cla: yes label Oct 15, 2016

vicb added action: review The PR is still awaiting reviews from at least one requested reviewer area: core Issues related to the framework runtime labels Oct 15, 2016

jeffbcross previously requested changes Oct 24, 2016

View reviewed changes

jeffbcross self-assigned this Oct 24, 2016

jeffbcross mentioned this pull request Oct 24, 2016

<html> component angular/universal#533

Closed

alfaproject reviewed Oct 25, 2016

View reviewed changes

PatrickJS mentioned this pull request Oct 31, 2016

Support rendering just a single component rather than a complete HTML document angular/universal#347

Closed

mklemme mentioned this pull request Oct 31, 2016

Support Rendering Server Side? vinaygopinath/ng2-meta#6

Open

vicb reviewed Nov 10, 2016

View reviewed changes

vicb added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews pr_state: LGTM and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Nov 10, 2016

vicb reviewed Nov 11, 2016

View reviewed changes

vicb reviewed Dec 6, 2016

View reviewed changes

vicb reviewed Dec 7, 2016

View reviewed changes

feat(platform browser): introduce Meta service

c9c22b3

vicb added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Dec 9, 2016

vicb merged commit 72361fb into angular:master Dec 9, 2016

kodeine mentioned this pull request Jan 27, 2017

meta data readable by Facebook/Twitter? fulls1z3/ngx-meta#13

Closed

sdsanders mentioned this pull request Sep 7, 2017

Migrate to the Angular 4 metaService sdsanders/episode-ninja-web#13

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 11, 2019

feat(platform browser): introduce Meta service #12322

feat(platform browser): introduce Meta service #12322

Uh oh!

Conversation

DzmitryShylovich commented Oct 15, 2016

Uh oh!

DzmitryShylovich commented Oct 15, 2016

Uh oh!

jeffbcross left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FrozenPandaz Oct 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffbcross commented Oct 24, 2016

Uh oh!

lacolaco commented Oct 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zoechi commented Oct 30, 2016

Uh oh!

jeffbcross commented Oct 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeffbcross commented Oct 31, 2016

Uh oh!

kodeine commented Nov 5, 2016

Uh oh!

DzmitryShylovich commented Nov 6, 2016

Uh oh!

kodeine commented Nov 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DzmitryShylovich commented Nov 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DzmitryShylovich Nov 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffbcross left a comment •

edited

Loading

FrozenPandaz Oct 24, 2016 •

edited

Loading

lacolaco commented Oct 25, 2016 •

edited

Loading

jeffbcross commented Oct 31, 2016 •

edited

Loading

DzmitryShylovich Nov 11, 2016 •

edited

Loading

DzmitryShylovich Nov 11, 2016 •

edited

Loading