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

Don't update document.currentScript for module scripts #1000

Merged
merged 2 commits into from
Apr 15, 2016

Conversation

domenic
Copy link
Member

@domenic domenic commented Apr 5, 2016

Closes #997.

@annevk
Copy link
Member

annevk commented Apr 8, 2016

See #997 (comment) for feedback.

@domenic domenic added the do not merge yet Pull request must not be merged per rationale in comment label Apr 8, 2016
@domenic domenic self-assigned this Apr 8, 2016
@domenic domenic force-pushed the no-module-currentscript branch from ff9cf1d to 33ea3ae Compare April 11, 2016 19:25
@domenic
Copy link
Member Author

domenic commented Apr 11, 2016

Updated

@domenic domenic removed the do not merge yet Pull request must not be merged per rationale in comment label Apr 11, 2016
@domenic domenic assigned annevk and unassigned domenic Apr 11, 2016
@annevk
Copy link
Member

annevk commented Apr 13, 2016

I think if the module script is the reentrant script we'd want it to be set to null too, no? Or can that never happen with module scripts?

@domenic
Copy link
Member Author

domenic commented Apr 13, 2016

Are you talking about updating the non-normative note?

@annevk
Copy link
Member

annevk commented Apr 13, 2016

I meant that rather than only updating it for classic scripts, we should update it for module scripts too, but just set it to null, and once the script is executed restore the previous value.

@domenic
Copy link
Member Author

domenic commented Apr 13, 2016

Oh, interesting. I felt like just ignoring currentScript would be better, but that's probably reasonable behavior too.

I think reentrancy can happen for async module scripts with no dependencies.

@annevk
Copy link
Member

annevk commented Apr 13, 2016

We should probably also add a note explaining why it's null.

@domenic domenic assigned domenic and unassigned annevk Apr 13, 2016
@domenic domenic force-pushed the no-module-currentscript branch from 33ea3ae to 456c115 Compare April 13, 2016 16:57
@domenic
Copy link
Member Author

domenic commented Apr 13, 2016

Pushed the new set-to-null semantics.

I'm not sure what kind of note we could add, besides "because we don't like this API any more". I'd welcome a suggestion.

@domenic domenic assigned annevk and unassigned domenic Apr 13, 2016
@annevk
Copy link
Member

annevk commented Apr 14, 2016

I guess we could remove the "domintro" box, mark it "// historical" in IDL, and add "This API fell out of fashion as it globally exposes script and SVG script element. The JavaScript community is looking for a way to identify these without making them globally available." as a note underneath the normative definition.

@domenic
Copy link
Member Author

domenic commented Apr 14, 2016

I think that's not a great idea verbatim, as whatever we come up with will only work for module scripts anyway. Maybe... keep domintro, mark it // classic scripts only in IDL, and add "This API fell out of fashion as it globally exposes script and SVG script elements. As such, it is not available in newer contexts, such as when running module scripts or when running scripts in a shadow tree. The JavaScript community is looking into a new solution for identifying the running script in such contexts, which does not make it globally available."

@annevk
Copy link
Member

annevk commented Apr 14, 2016

"classic scripts in a document only" then if we're mentioning shadow trees. But that seems fine. Do you want me to patch things up?

@domenic
Copy link
Member Author

domenic commented Apr 14, 2016

I can do it when I get to work.

domenic added 2 commits April 14, 2016 15:14
They set document.currentScript to null, and no longer fire
beforescriptexecute and afterscriptexecute events.

Closes #997. See #1013 for a future alternative to
document.currentScript, and #943 for more discussion of the events.
@domenic domenic force-pushed the no-module-currentscript branch from 456c115 to 6ddf1fa Compare April 14, 2016 19:21
@domenic
Copy link
Member Author

domenic commented Apr 14, 2016

Added a fixup commit

@annevk annevk merged commit 6918282 into master Apr 15, 2016
@annevk annevk deleted the no-module-currentscript branch April 15, 2016 06:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants