Skip to content
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.

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

Already on GitHub? Sign in to your account

馃悰loader: Disable caching of LoaderService Promise #24939

Merged
merged 4 commits into from Oct 8, 2019
Merged

馃悰loader: Disable caching of LoaderService Promise #24939

merged 4 commits into from Oct 8, 2019

Conversation

mattwomple
Copy link
Member

@mattwomple mattwomple commented Oct 7, 2019

Cached LoaderService becomes invalid on page transition in a PWA. Rely
on .installExtensionForDoc() to short-circuit quickly when the service
is already available and to install the doc extension when needed.

Fixes #24740

In a PWA, the loaderServicePromise cannot be reused across page
navigations; style[amp-extension="amp-loader"] is lost. Add a dispose
method to reset the promise when shadowDoc.close() runs.
@mattwomple
Copy link
Member Author

/CC: @sparhami

@sparhami
Copy link

sparhami commented Oct 7, 2019

Hmm, I think we should just remove all the caching of the Promise, and just return it directly. the functions for installing the extension and getting the service short circuit fairly quickly anyway.

@sparhami
Copy link

sparhami commented Oct 7, 2019

Also, I think on dispose is not quite the right thing, since you could have two shadow AMP documents active at once.

@mattwomple
Copy link
Member Author

Thanks for the feedback!

Hmm, I think we should just remove all the caching of the Promise, and just return it directly.

Ok, I'm happy to give this a go. I also recognize I hijacked this issue from you and am ok if you want to take it over. I just got curious how the loader integrates and wanted to dig in a bit.

since you could have two shadow AMP documents active at once

Really? Is this a reality or a hope for someday in the future? Last I tried several months ago, this was not possible. I'll be really excited if two AMP docs can coexist without detrimental cross-talk.

@sparhami
Copy link

sparhami commented Oct 7, 2019

Thanks for the feedback!

Hmm, I think we should just remove all the caching of the Promise, and just return it directly.

Ok, I'm happy to give this a go. I also recognize I hijacked this issue from you and am ok if you want to take it over. I just got curious how the loader integrates and wanted to dig in a bit.

since you could have two shadow AMP documents active at once

Really? Is this a reality or a hope for someday in the future? Last I tried several months ago, this was not possible. I'll be really excited if two AMP docs can coexist without detrimental cross-talk.

Thanks for the feedback!

Hmm, I think we should just remove all the caching of the Promise, and just return it directly.

Ok, I'm happy to give this a go. I also recognize I hijacked this issue from you and am ok if you want to take it over. I just got curious how the loader integrates and wanted to dig in a bit.

Feel free to continue on this change if you have time. If you don't, I can send out a PR.

since you could have two shadow AMP documents active at once

Really? Is this a reality or a hope for someday in the future? Last I tried several months ago, this was not possible. I'll be really excited if two AMP docs can coexist without detrimental cross-talk.

I think with amp-next-page it can have multiple shadow docs on the page at once. I'm no sure what bugs might come up as a result. I haven't looked into it for a while though.

Cached LoaderService becomes invalid on page transition in a PWA. Rely
on .installExtensionForDoc() to short-circuit quickly when the service
is already available and to install the doc extension when needed.
@mattwomple mattwomple changed the title 馃悰loader: Add dispose method 馃悰loader: Disable caching of LoaderService Promise Oct 8, 2019
@sparhami
Copy link

sparhami commented Oct 8, 2019

@choumx Can you approve this PR for the owners check?

@sparhami sparhami merged commit 6bbca60 into ampproject:master Oct 8, 2019
@mattwomple mattwomple deleted the patch-3 branch October 9, 2019 05:44
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.

Investigate amp-loader CSS in shadow docs
4 participants