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

fix: reapply Safari 15 min #906

Merged
merged 1 commit into from
May 30, 2023
Merged

fix: reapply Safari 15 min #906

merged 1 commit into from
May 30, 2023

Conversation

wegry
Copy link
Contributor

@wegry wegry commented May 8, 2023

Fix for #904. Our usage of CJS inside of ESM exports seems to generate a ReferenceError with our stack in Safari 15 when coupled with a class that's being assigned to module.exports within a ud defn function.

Swapping our browserslist default creates quite a bit of noise in our error logging, as it changes stack trace locations and increases stack size. It also increases our output code size by roughly 10%. edit: bundlephobia doesn't rounds the two output sizes together, but we're in the ~1% range minified.

Longer term, I'd like to remove the remainder of ud in its entirety as it doesn't work with TS and is thus blocking moving off flow. See #910.

Copy link
Contributor

@dmy7r0 dmy7r0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to test this against Safari 15 and it is not breaking anymore.

Sidenote: It took me a decent amount of time to set up the env for testing and I wish it was simpler/automated in repetitive tasks.

@Macil
Copy link
Member

Macil commented May 26, 2023

I'm very worried that this ud call isn't the only thing that could trigger this issue. If we switch to target Safari 15, then future code changes could hit this issue again.

I found an issue about the underlying Safari bug: babel/babel#14289. It looks like the recommended workaround is to manually enable the @babel/plugin-proposal-class-properties babel plugin.

@wegry wegry force-pushed the fix/reapply-safari-15-min branch from 08f447e to eeabdde Compare May 26, 2023 17:18
Our usage of CJS inside of ESM exports seems to generate a ReferenceError with our stack in Safari 15 when coupled with a class that's being assigned to `module.exports` within a ud defn function.
Swapping our browserslist default creates quite a bit of noise in our error logging, as it changes stack trace locations and increases stack size. It also increases our output code size by roughly 1%.
@wegry wegry force-pushed the fix/reapply-safari-15-min branch from eeabdde to d297787 Compare May 26, 2023 17:22
@wegry
Copy link
Contributor Author

wegry commented May 26, 2023

I found an issue about the underlying Safari bug: babel/babel#14289. It looks like the recommended workaround is to manually enable the @babel/plugin-proposal-class-properties babel plugin.

@Macil see d297787.

@wegry wegry merged commit 53a2f96 into main May 30, 2023
6 checks passed
@wegry
Copy link
Contributor Author

wegry commented May 30, 2023

@Macil https://github.com/terser/terser/blob/master/CHANGELOG.md#v5176 may hint at the heart of the issue that underlies this patch. It could also explain why we hadn't necessarily seen the issue manifest in our downstream projects.

@wegry wegry deleted the fix/reapply-safari-15-min branch May 30, 2023 19:00
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.

None yet

3 participants