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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] dynamic-import importHref() callback fix when the same resource is requested multiple times #4114

Closed
wants to merge 14 commits into from

Conversation

gronke
Copy link
Contributor

@gronke gronke commented Oct 30, 2016

fixes #3814

The expected behavior of importHref() is that onload or onerror get called once the resource is loaded.

When requesting a resource for the second time, we can immediately call the onload or onerror function in case it was already completed. Otherwise we attach to the same events as the previous request.

Unit Test

@gronke gronke force-pushed the extend-tests-dynamic-import branch from 92ebfd0 to 977a93a Compare October 30, 2016 18:10
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@gronke gronke force-pushed the extend-tests-dynamic-import branch from fe43d65 to 3f30a44 Compare October 30, 2016 19:08
@googlebot
Copy link

CLAs look good, thanks!

@gronke gronke changed the title extend dynamic-import tests dynamic-import importHref() callback fix when the same resource is requested multiple times Oct 30, 2016
Copy link
Contributor

@sorvell sorvell left a comment

Choose a reason for hiding this comment

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

Would you mind addressing the feedback and updating the PR so it can be merged cleanly? Thanks for the PR!

var imprt = Polymer.Base.importHref(url);

var onLoad = function() {
document.removeEventListener('load', onLoad);
Copy link
Contributor

Choose a reason for hiding this comment

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

document here should be imprt

@@ -0,0 +1,24 @@
<!DOCTYPE html>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove if this file is not used anywhere.

@gronke
Copy link
Contributor Author

gronke commented Nov 3, 2016

@sorvell thanks for the review. all done!

@blasten
Copy link
Contributor

blasten commented Nov 3, 2016

oh this would have also fixed #4124

@gronke
Copy link
Contributor Author

gronke commented Nov 3, 2016

@blasten will rebase

Update: @blasten Your changes are applied to this branch. In fact your fix catched one more bug by always removing both event listeners.

@gronke gronke force-pushed the extend-tests-dynamic-import branch from e3e408a to 6cc2451 Compare November 3, 2016 19:12
@gronke gronke force-pushed the extend-tests-dynamic-import branch from 6cc2451 to 3eb6336 Compare November 3, 2016 19:16
@gronke gronke force-pushed the extend-tests-dynamic-import branch from 3eb6336 to 9146ec2 Compare November 3, 2016 21:11
@gronke gronke force-pushed the extend-tests-dynamic-import branch 2 times, most recently from 118fc39 to a2c4e33 Compare November 8, 2016 01:35
@gronke gronke changed the title dynamic-import importHref() callback fix when the same resource is requested multiple times [WIP] dynamic-import importHref() callback fix when the same resource is requested multiple times Nov 12, 2016
@gronke gronke force-pushed the extend-tests-dynamic-import branch from a2c4e33 to eade3d2 Compare November 25, 2016 05:32
@gronke gronke force-pushed the extend-tests-dynamic-import branch from 247e88a to 1fec33b Compare December 5, 2016 22:49
@gronke
Copy link
Contributor Author

gronke commented Dec 9, 2016

Closing in favor of #4209

@gronke gronke closed this Dec 9, 2016
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.

importHref only triggers onload and onerror if first invocation has listeners
4 participants