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
Handle inline scripts when systemjs itself is inline #2369
base: main
Are you sure you want to change the base?
Conversation
File size impactMerging main into main will impact 2 files in 1 group. browser (2/2)
node (0/1)No impact on files in node group. extras (0/8)No impact on files in extras group. |
Done |
The build failed but it seems unrelated, I think it would pass if restarted but I don't have the rights to restart the Travis build. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty good to me. Can you think of a way to add tests? Seems to be hard since the tests are all run in the same HTML file that loads systemjs via <script src>
rather than inline.
The lastScript
logic that's already present in systemjs doesn't seem to be the best approach, since multiple inline scripts would have the same lastAutoImportUrl logic in them. Using document.currentScript
would be better, but isn't supported in IE11. I wonder if we could use document.scripts[document.scripts.length - 1]
rather than the current approach? Got that idea from https://makandracards.com/makandra/497684-emulating-document-currentscript-in-old-browsers. If we can switch to that, I think it would be better.
src/features/script-load.js
Outdated
} | ||
else { | ||
inlineScriptCount++ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inlineScriptCount++ | |
inlineScriptCount++; |
src/features/script-load.js
Outdated
systemJSPrototype.register = function (deps, declare) { | ||
if (hasDocument && document.readyState === 'loading' && typeof deps !== 'string') { | ||
var scripts = document.querySelectorAll('script[src]'); | ||
var lastScript = scripts[scripts.length - 1]; | ||
var lastAutoImportUrl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you move this inside of register? To avoid race conditions with multiple scripts being executed in the same tick of the event loop? I didn't think it would be possible for two inline scripts to execute before a setTimeout tick occurs.
src/features/script-load.js
Outdated
systemJSPrototype.register = function (deps, declare) { | ||
if (hasDocument && document.readyState === 'loading' && typeof deps !== 'string') { | ||
var scripts = document.querySelectorAll('script[src]'); | ||
var lastScript = scripts[scripts.length - 1]; | ||
var lastAutoImportUrl; | ||
lastAutoImportDeps = deps; | ||
if (lastScript) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the ideal implementation would use the src
if it's present and the __inline_script__
if it's not. Something like this:
if (lastScript) { | |
if (lastScript.src) { |
Thanks for the review, for now I don't have time to handle your feedback but I'll come back to this PR when possible ;) |
I have just pushed a commit to address the review. About how to test this scenario where systemjs itself is inlined:
But I would like your approval about this strategy before writing it. |
Description
When systemjs is inlined in the HMTL file, it won't execute inline scripts
This pull request should fix this use case, it is a follow up of #2357