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 add "use strict" to external libraries #3746

Merged
merged 2 commits into from
Dec 6, 2022
Merged

Conversation

wch
Copy link
Collaborator

@wch wch commented Dec 5, 2022

This fixes an issue in selectize-plugin-a11y.min.js introduced by #3709. That PR added a "use strict" to the top of selectize-plugin-a11y.min.js, probably due to a change in the version of esbuild. It resulted in this error in the JS console when a selectize input is clicked:

selectize-plugin-a11y.min.js:1 Uncaught ReferenceError: i is not defined
    at selectize-plugin-a11y.min.js:1:677
    at Array.forEach (<anonymous>)
    at MutationObserver.<anonymous> (selectize-plugin-a11y.min.js:1:493)

Even with this error, the graphical UI of the selectize input still worked, but there were probably issues with accessibility.

@wch wch requested a review from cpsievert December 5, 2022 22:30
@cpsievert
Copy link
Collaborator

This change should probably live in tools/selectize-patches (since running tools/updateSelectize.R would overwrite this).

More generally though, it is a bit scary to me that we're adding "use strict" to other people's legacy JS code (a quick grep for for (i returns some results back, so if we do adopt "use strict", it seems we should making more changes then just this one).

I'd almost rather not add the "use strict" for now, especially if it's easy to opt-out of it

@wch
Copy link
Collaborator Author

wch commented Dec 5, 2022

I believe "use strict" started showing up in the minified files because esbuild (and/or various esbuild plugins) were updated in #3709.

@schloerke Do you know if there's a way to get esbuild to stop emitting "use strict" in the minified code? I think esbuild shouldn't be adding it unless it was present in the original code (when doing JS to JS compilation; TS to JS is a different story).

@cpsievert

This comment was marked as outdated.

@cpsievert cpsievert changed the title Add missing var in loop Don't add "use strict" to external libraries Dec 6, 2022
@cpsievert cpsievert merged commit ffb6736 into main Dec 6, 2022
@cpsievert cpsievert deleted the fix-missing-var branch December 6, 2022 20:17
This pull request was closed.
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.

2 participants