Skip to content

Use 'document.scripts' instead of document.getElementsByTagName #496

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

Merged
merged 2 commits into from
Sep 1, 2016

Conversation

perry-mitchell
Copy link
Contributor

@perry-mitchell perry-mitchell commented May 31, 2016

document.scripts preserves script-addition order, so we can add scripts in the HEAD as well as BODY.

Currently using getElementsByTagName fails if the script is placed in the HEAD.

EDIT: This is nicely discussed here, where the author mentions (discussing the getElementsByTagName alternative):

There are two situations where this snippet will fail, though. When a script is loaded asynchronously using <script async>, the order of script execution is uncertain. Therefore the document.getElementsByTagName() trick will not work, as it relies on the currently executed script being the last of that collection. The same holds true for <script>s that are not appended to the end of the document. In the future this might become less of a problem, as document.currentScript properly resolves async scripts.

@SpaceK33z
Copy link
Member

Hi, thanks for this PR. A lot of people have this issue, also see #117. Your PR is almost there, but could you make it to work like this:

  1. Use document.currentScript if available. This is the most reliable way to get the current script, and works in most browsers.
  2. Fallback to document.scripts.
  3. If that fails, throw an exception.

@perry-mitchell
Copy link
Contributor Author

Thanks @SpaceK33z - Will update this as soon as I can 🙂

@perry-mitchell
Copy link
Contributor Author

@SpaceK33z I've updated the PR - using document.currentScript as you said.

@SpaceK33z SpaceK33z merged commit 153448d into webpack:master Sep 1, 2016
@SpaceK33z
Copy link
Member

Thank you! I have some tiny nitpicks, but I'll fix those in a separate commit.

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

Successfully merging this pull request may close these issues.

2 participants