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

Modules executing in reverse post-order #6261

Closed
guybedford opened this issue Aug 31, 2019 · 7 comments · Fixed by #6295
Closed

Modules executing in reverse post-order #6261

guybedford opened this issue Aug 31, 2019 · 7 comments · Fixed by #6295

Comments

@guybedford
Copy link

guybedford commented Aug 31, 2019

Module execution of ES modules is based on post-order execution up the graph (with circular deduping).

The Edge behaviour is incorrectly implementing a reverse post-order execution.

The simple example of this is the following:

<script>order=[]</script>
<script type="module">
import '/a.js';
import '/b.js';
console.log(order);
</script>

where a.js contains order.push('a') and b.js similarly.

where edge will execute b and then a, instead of a and then b!

The fix should be simple (one loop can just be reversed somewhere), but this is a really important bug to fix as many projects will write code like the following:

import 'polyfill';
import 'stuff-that-needs-polyfill';

and without these guaranatees those code paths break down.

I would highly recommend marking this as an urgent priority.

@guybedford
Copy link
Author

The scenario here might be a little more complex actually. For example, the following code executes dependencies in indeterminate order:

<!doctype html>
<script>
function blobUrl (source) {
  return URL.createObjectURL(new Blob([source], { type: 'application/javascript' }));
}

window.order = [];
document.head.appendChild(Object.assign(document.createElement('script'), {
  type: 'module',
  innerHTML: `
    import '${blobUrl('order.push("a")')}';
    import '${blobUrl('order.push("b")')}';
    console.log(order);
  `
}));
</script>

when the ordering always should be [a,b], it will vary between that and [b,a] roughly 50% of the time.

@guybedford
Copy link
Author

On the other hand, the following always executes in the correct order:

<!doctype html>
<script>
window.order = [];
document.head.appendChild(Object.assign(document.createElement('script'), {
  type: 'module',
  innerHTML: `
    import 'data:application/javascript,order.push("a")';
    import 'data:application/javascript,order.push("b")';
    console.log(order);
  `
}));
</script>

It seems to me like request timings or otherwise are possibly being allowed to affect execution order.

@fatcerberus
Copy link
Contributor

Is there a specific loading order guaranteed by the specification?

@rhuanjl
Copy link
Collaborator

rhuanjl commented Sep 2, 2019

@fatcerberus yes there is.

@guybedford the module load order was wrong in versions 1.8 and 1.9 of CC it should have been fixed for 1.10 by this PR of mine and I believe that the most recent version of Edge should be using 1.10 or even 1.11 #5238

The issue with indeterminate order sounds to me like a problem with Edge not CC based on my knowledge of the CC code base but as an external contributor to CC I’ve not seen any of the Edge code so can’t say for sure.

@guybedford
Copy link
Author

@rhuanjl I was testing on Edge 44.18362. If you think the out-of-order issue will have been resolved since then that is great to hear.

@rhuanjl
Copy link
Collaborator

rhuanjl commented Sep 2, 2019

I’m afraid I don’t know which version of CC each version of Edge uses; will need one of the Microsoft guys to comment.

@rhuanjl
Copy link
Collaborator

rhuanjl commented Sep 26, 2019

Taking another look at this - I'm wrong above. CC module load order remains wrong in the latest version. I fixed one error with my previous PR (execution of the list of modules backwards) but there were two errors.

The list of child modules is being stored internally in an internal data structure type called a BaseDictionary - which uses Key, Value pairs and is sorted based on the keys - in this case the specifiers, so execution order is dependent on the specifier - the test case I added before had conveniently alphabetical module names so appeared to work.

I've opened a PR that should fix this remaining issue. I'm sorry that I was wrong above.

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

Successfully merging a pull request may close this issue.

3 participants