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

Changing System.resolve to be synchronous. Resolves #1985. #1996

Merged
merged 8 commits into from Aug 25, 2019

Conversation

@joeldenning
Copy link
Collaborator

commented Aug 23, 2019

See #1985

Copy link
Member

left a comment

Perfect, thanks so much for putting this one together.

This should really be a major, but I wonder if we can get away with a minor?

src/system-core.js Outdated Show resolved Hide resolved
src/features/import-map.js Outdated Show resolved Hide resolved
src/features/import-map.js Outdated Show resolved Hide resolved
@joeldenning

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 24, 2019

@guybedford just updated this with your feedback

@joeldenning

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 24, 2019

It is a breaking change and I know of one codebase that will break because of it (assumes System.resolve returns a promise). However, I don't know how common it is for people to rely on the Promise being returned -- we fairly recently switched it from synchronous to promise based.

});
return Promise.all(Array.prototype.map.call(document.querySelectorAll('script[type="systemjs-importmap"]'), function (script) {
return (script._j || script.src && fetch(script.src).then(function (resp) {return resp.json()}) || Promise.resolve(JSON.parse(script.innerHTML)))
.then(function (json) {

This comment has been minimized.

Copy link
@joeldenning

joeldenning Aug 24, 2019

Author Collaborator

Wait is there maybe a bug here -- a race condition?

I think we're applying the import maps in the order that we can load them instead of document order. Right? I'll look into a fix for it.

This comment has been minimized.

Copy link
@joeldenning

joeldenning Aug 24, 2019

Author Collaborator

Just fixed this ^^ with a new commit.

joeldenning added 3 commits Aug 24, 2019
@guybedford

This comment has been minimized.

Copy link
Member

commented Aug 25, 2019

I added some minor adjustments in 6eb52d1 750f309.

Thanks for this, looks great. Agreed on the major. Would be worth having a nice release blog post too.

@guybedford guybedford force-pushed the issue-1985 branch from 6eb52d1 to 750f309 Aug 25, 2019
@guybedford guybedford merged commit 3b62497 into master Aug 25, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@guybedford guybedford deleted the issue-1985 branch Aug 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.