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

lastScript detection doesn't seem to be 100% reliable #2465

Open
jchip opened this issue Aug 7, 2023 · 3 comments · May be fixed by #2469
Open

lastScript detection doesn't seem to be 100% reliable #2465

jchip opened this issue Aug 7, 2023 · 3 comments · May be fixed by #2469

Comments

@jchip
Copy link

jchip commented Aug 7, 2023

Demonstration

when loading two systemjs files:

  <script src="a.js"></script>
  <script src="b.js"></script>

occasionally, a.js's dependencies loading occurs before b.js register, and the lastScript detection would fail to get the correct URL.

document.currentScript would be reliable, but it's not supported on IE, where there's a polyfill here https://github.com/JamesMGreene/document.currentScript

Code Sandbox:

Expected Behavior

100% reliable script URL detection

Actual Behavior

occasionally script URL detection gets the dependencies of a previous bundle being loaded.

@guybedford
Copy link
Member

@jchip this should be reliable, if it is not that is a bug. There were a bunch of refactorings to this code in eg #2116 and #2114.

If you would like to work on a fix that would be very welcome.

@jchip
Copy link
Author

jchip commented Aug 14, 2023

@guybedford I observed this periodically. It happens a little more as the number of <script> tags increases. Looking at James Greene's work https://github.com/JamesMGreene/document.currentScript and https://github.com/JamesMGreene/currentExecutingScript it seems we can improve this a bit.

I think a potential change could be:

  1. Use document.currentScript https://developer.mozilla.org/en-US/docs/Web/API/Document/currentScript
  2. Add a getCurrentScript hook in case someone wants to provide different logic, such as use James' currentExecutingScript
  3. Add documentation to note this

A sample proposal for 1 and 2:

systemJSPrototype.getCurrentScript = function () {
  if (hasDocument && document.readyState === 'loading') {
    if (document.currentScript) {
      return document.currentScript;
    }
    var scripts = document.querySelectorAll('script[src]');
    return scripts[scripts.length - 1];
  }
  return undefined;
};

@guybedford
Copy link
Member

I'm open to it - this project once had to support IE, so the constraints may have changed. It's important that whatever is done is tested and correct though!

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.

2 participants