Skip to content

♻️Move VariableService to doc level #2#22300

Merged
calebcordry merged 2 commits intoampproject:masterfrom
calebcordry:vars-doc-level
May 17, 2019
Merged

♻️Move VariableService to doc level #2#22300
calebcordry merged 2 commits intoampproject:masterfrom
calebcordry:vars-doc-level

Conversation

@calebcordry
Copy link
Copy Markdown
Member

@calebcordry calebcordry commented May 14, 2019

Reimplementing this refactor after adding shadow integration test.

Closes #22217

calebcordry
calebcordry commented May 14, 2019
View reviewed changes
extensions/amp-analytics/0.1/amp-analytics.js
AMP.registerServiceForDoc('activity', Activity);
installVariableService(AMP.win);
installLinkerReaderService(AMP.win);
AMP.registerServiceForDoc('amp-analytics-variables', VariableService);
Copy link
Copy Markdown
Member Author

@calebcordry calebcordry May 14, 2019

Choose a reason for hiding this comment

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

This is where the previous PR was broken. It was written as installVariableServiceForDoc(AMP.ampdoc); and AMP.ampdoc was not doing the right thing in the shadow case.

Copy link
Copy Markdown
Contributor

@torch2424 torch2424 May 15, 2019

Choose a reason for hiding this comment

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

Thanks for the heads up 😄

Copy link
Copy Markdown
Contributor

@lannka lannka May 16, 2019

Choose a reason for hiding this comment

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

so should we remove installVariableServiceForDoc if it's dangerous?

Copy link
Copy Markdown
Member Author

@calebcordry calebcordry May 16, 2019

Choose a reason for hiding this comment

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

I kind of did? It is now installVariableServiceForTesting and only used in tests.

torch2424
torch2424 approved these changes May 15, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@torch2424 torch2424 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

extensions/amp-analytics/0.1/amp-analytics.js
AMP.registerServiceForDoc('activity', Activity);
installVariableService(AMP.win);
installLinkerReaderService(AMP.win);
AMP.registerServiceForDoc('amp-analytics-variables', VariableService);
Copy link
Copy Markdown
Contributor

@torch2424 torch2424 May 15, 2019

Choose a reason for hiding this comment

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

Thanks for the heads up 😄

@calebcordry calebcordry merged commit 182498b into ampproject:master May 17, 2019
@calebcordry calebcordry deleted the vars-doc-level branch May 17, 2019 22:05
@mattwomple
Copy link
Copy Markdown
Member

mattwomple commented May 17, 2019

@calebcordry Did you test this with the original example I shared with you in #22137 (direct link to comment)? When I navigate to the second page in the PWA, I see error: Expected service amp-analytics-variables to be registered. Full stack trace:

log.js:184 [Resource] failed to build: amp-analytics#1 Error: Expected service amp-analytics-variables to be registered
    at Log.assert (log.js:342)
    at devAssert (log.js:752)
    at getServiceInternal (service.js:383)
    at getServiceForDoc (service.js:245)
    at variableServiceForDoc (variables.js:317)
    at AmpAnalytics.buildCallback (amp-analytics.js:124)
    at custom-element.js:513
    at new Promise (<anonymous>)
    at HTMLElement.build (custom-element.js:510)
    at Resource.build (resource.js:337)

@calebcordry
Copy link
Copy Markdown
Member Author

calebcordry commented May 17, 2019

@mattwomple I was just going to ask you to test, I tested a few different shadow viewers but I did not see this issue. Admittedly I did not try your example. Looking now.

@calebcordry calebcordry mentioned this pull request May 18, 2019
Merged
@ampprojectbot ampprojectbot mentioned this pull request May 7, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lannka lannka Awaiting requested review from lannka

@zhouyx zhouyx Awaiting requested review from zhouyx

1 more reviewer

@torch2424 torch2424 torch2424 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

🐛Analytics vars refactor breaks shadow dom

5 participants

@calebcordry @mattwomple @torch2424 @lannka @googlebot