-
Notifications
You must be signed in to change notification settings - Fork 375
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
Avoid document write in single script linker loading #9814
Conversation
jnehlmeier or anyone from GWT, any feedback on this PR? |
@@ -119,6 +104,7 @@ function __MODULE_FUNC__() { | |||
return (slashIndex >= 0) ? path.substring(0, slashIndex + 1) : ''; | |||
}; | |||
|
|||
var thisScript = $doc.currentScript; |
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.
According to caniuse.com currentScript
is not supported by IE 11. So you can not use it for now unless a decision has been made to drop IE 11 in the next GWT release.
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.
Are you OK to make the code try currentScript and if fails to try the old implementation?
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.
That seems reasonable to me - put an explicit note so that when IE 11 is finally truly dead we know that this can be simplified out?
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.
Are you OK to make the code try currentScript and if fails to try the old implementation?
Yes that would be fine.
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.
@Mivr do you want to make this change so we can see about merging?
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.
@niloc132 Yes, sorry for the delay I am just way too busy and warnings are not the highest priority
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.
Yep no problem, just checking in with various open PRs to see what we can land before trying to do a release. No rush, we've got other pending PRs, but this one is small enough we could ship without it if you don't get the chance to get to it.
- replaces PR gwtproject#9814 - due to currentScript not being fully supported, wrapped in conditional - fixes issue gwtproject#9797
Closing in favor of #9847 |
Avoid document write method in single script linker.
Fixes #9797