Skip to content

ampdoc: Relax usage requirements of getMetaByName#26729

Closed
mdmower wants to merge 1 commit intoampproject:masterfrom
mdmower:pr-ampdoc
Closed

ampdoc: Relax usage requirements of getMetaByName#26729
mdmower wants to merge 1 commit intoampproject:masterfrom
mdmower:pr-ampdoc

Conversation

@mdmower
Copy link
Copy Markdown
Contributor

@mdmower mdmower commented Feb 11, 2020

If <head> doesn't exist, don't throw, just return null.

If <head> doesn't exist, don't throw, just return null.
@mdmower
Copy link
Copy Markdown
Contributor Author

mdmower commented Feb 11, 2020

I'm not actually sure I really want this, but it does reduce the requirements on unit tests.

*/
getMetaByName(name) {
if (!name) {
if (!name || !this.win.document || !this.win.document.head) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this.win.document be null?

Copy link
Copy Markdown
Member

@samouri samouri Feb 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On top of that, how could document.head be null/undefined? I thought even an html document without a <head> tag will still be given an empty one.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In practice, this is only for unit tests. See for example

, where the document object is defined by hand. I'll defer to you two on whether this change should be merged; I'm impartial.

@mdmower
Copy link
Copy Markdown
Contributor Author

mdmower commented Feb 12, 2020

Choosing to abandon. Updated unit tests in #26687 .

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants