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

BUG: Dynamic import should return different promise objects, even if the specifier is the same. #5795

Closed
rwaldron opened this issue Oct 25, 2018 · 3 comments · Fixed by #5803
Labels
Milestone

Comments

@rwaldron
Copy link

Test: https://github.com/tc39/test262/blob/master/test/language/expressions/dynamic-import/always-create-new-promise.js

Result:

the returned promises are not the same, regardless the reference and specifier pair Expected SameValue(«[object Promise]», «[object Promise]») to be false

@rhuanjl
Copy link
Collaborator

rhuanjl commented Oct 25, 2018

Hmm currently internally the promise for a dynamic import is a property of the SourceTextModuleRecord object - if you import the same module a second time the SourceTextModuleRecord object from the first import() is retrieved and the promise from it is returned.

This means that when the import() completes it has an easy reference to the promise it needs to resolve, so to fix this would probably have to change this mechanism - perhaps introducing a list of promises.

@fatcerberus
Copy link
Contributor

It might not be necessary to keep a list; CC could keep the single internal promise and wire up the ones returned from import to it. This way you resolve the internal one and all the ones attached to it get resolved automatically.

@rhuanjl
Copy link
Collaborator

rhuanjl commented Oct 29, 2018

I've submitted a fix, this commit 866386b in PR 5803.

It adds the new promises as reactions to the existing promise with an internal equivalent of promiseObject.then(a => a, e => throw e);

@MikeHolman MikeHolman added the Bug label Oct 31, 2018
@MikeHolman MikeHolman added this to the vNext milestone Oct 31, 2018
chakrabot pushed a commit that referenced this issue Nov 1, 2018
Merge pull request #5803 from rhuanjl:moduleFixes

This PR fixes a collection of module related issues - some of these are very small so I thought better to combine them into one PR. I hope this isn't too much at once.

1. Restore the use of relative paths for imports within modules in ch - this was implemented a long time ago and then broken - the functionality was mostly still there just broken in a few places - originally this was issue 3257, it also has a newer issue I opened 5237. As well as restoring the functionality I restored the test case from 3257 which was no longer testing what it had been designed for after a previous edit.

2. Don't print messages about failing to load modules or syntax errors in modules when processing dynamic imports - this was purely a ch issue but was problematic for several test262 cases per issue 5796.

3. Reject the promise from import() if a child module of the dynamically imported module throws a runtime error - there was a bug where this promise was never being resolved - this is also part of 5796.

4. `new import(anything)` should be a syntax error - 5797

5. import() should return a different promise each time even when using the same specifier - fix this by using JavascriptPromise::CreatePassThroughPromise 5795

cc @rwaldron @boingoing

fix #5796
fix #5795
fix #5797
fix #5237
re-fix #3257
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 a pull request may close this issue.

4 participants