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

Loader.install should resolve pending imports #48

Closed
matthewp opened this issue May 8, 2015 · 6 comments
Closed

Loader.install should resolve pending imports #48

matthewp opened this issue May 8, 2015 · 6 comments
Labels

Comments

@matthewp
Copy link

matthewp commented May 8, 2015

This was a bug in the old spec and appears to still be present. Consider this code:

loader.import("main.js");

setTimeout(function(){
  loader.install("main.js", value);
}, 5);

This creates a race condition where if main.js passes through resolve before loader.install is called it will continue through the loading steps and (I believe) fail an assertion in linking.

Is loader.cancel intended to fix this issue?

@matthewp
Copy link
Author

@caridy I think this should be labeled a bug. This problem means loader.install and loader.provide can only be safely used prior to any <module> tags or prior to any module loading having started. I'm not sure what their use-case would be if this were intentional and not a bug.

This same bug existed in the old System.set/System.define form and it made them effectively unusable.

@caridy caridy added bug and removed enhancement labels Aug 1, 2015
@caridy
Copy link
Contributor

caridy commented Aug 1, 2015

I will look into the details next week.

@dherman
Copy link

dherman commented Oct 9, 2015

This may have been fixed along the way as @caridy has been cleaning up the pipeline logic. He's currently working on a refactor of how the pipeline is represented (see PR #91). The semantics should be carefully ensuring there's only ever one promise stored for each stage, and we should only ever cancel an earlier stage's promise after a later stage has already been resolved. So I don't see off the top of my head how this bug would come up. But let's revisit it after the PR lands.

@caridy
Copy link
Contributor

caridy commented Dec 1, 2015

@matthewp this is now fixed (after PR #97).

@caridy caridy closed this as completed Dec 1, 2015
@matthewp
Copy link
Author

matthewp commented Dec 1, 2015

You meant "now fixed", right?

@caridy
Copy link
Contributor

caridy commented Dec 1, 2015

jejeje, yes! fixed.

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

No branches or pull requests

3 participants