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

Avoid document write in single script linker loading #9814

Closed
wants to merge 1 commit into from
Closed

Avoid document write in single script linker loading #9814

wants to merge 1 commit into from

Conversation

Mivr
Copy link

@Mivr Mivr commented Mar 27, 2023

Avoid document write method in single script linker.

Fixes #9797

@Mivr
Copy link
Author

Mivr commented Apr 4, 2023

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;
Copy link
Member

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.

Copy link
Author

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?

Copy link
Contributor

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?

Copy link
Member

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.

Copy link
Contributor

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?

Copy link
Author

@Mivr Mivr May 19, 2023

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

Copy link
Contributor

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.

jhickman added a commit to jhickman/gwt that referenced this pull request Aug 15, 2023
- replaces PR gwtproject#9814
 - due to currentScript not being fully supported, wrapped in conditional
- fixes issue gwtproject#9797
@niloc132
Copy link
Contributor

Closing in favor of #9847

@niloc132 niloc132 closed this Nov 15, 2023
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 this pull request may close these issues.

Replace document.write('<script ...>') with document.createElement('script')
3 participants