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

build(cdn): Ensure ES5 bundles do not use non-ES5 code #7550

Merged
merged 5 commits into from Mar 22, 2023
Merged

Conversation

mydea
Copy link
Member

@mydea mydea commented Mar 21, 2023

To avoid accidentally including non-ES5 code.

This does three things:

  • Add a validate:es5 command that checks that no non-ES5 syntax is in the ES5 bundle.
    • Note that this does not check for usage of ES6-only methods, e.g. arr.find()
  • Add a basic polyfill for the most important array/string ES6 methods that we have been using so far
  • Add an eslint rule that checks we don't use any non-polyfilled ES6 methods in the future

To avoid issues like #7541 in the future.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 21, 2023

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 20.57 KB (+0.65% 🔺)
@sentry/browser - ES5 CDN Bundle (minified) 64.19 KB (+1.06% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 19.13 KB (0%)
@sentry/browser - ES6 CDN Bundle (minified) 56.57 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 21.59 KB (0%)
@sentry/browser - Webpack (minified) 71.65 KB (0%)
@sentry/react - Webpack (gzipped + minified) 21.61 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified) 51.85 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 27.9 KB (+0.47% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 26.08 KB (-0.01% 🔽)
@sentry/replay ES6 CDN Bundle (gzipped + minified) 44.21 KB (+0.01% 🔺)
@sentry/replay - Webpack (gzipped + minified) 38.28 KB (0%)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 62.39 KB (-0.01% 🔽)
@sentry/browser + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 55.95 KB (+0.01% 🔺)

package.json Outdated Show resolved Hide resolved
@mydea mydea changed the title ci: Check es5 bundles for valid syntax build(cdn): Ensure ES5 bundles do not use non-ES5 code Mar 21, 2023
rollup/polyfills/es5.js Fixed Show fixed Hide fixed
@@ -0,0 +1,40 @@
// Sentry ES5 polyfills
Copy link
Member Author

Choose a reason for hiding this comment

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

My only concern here is that we may interfere with polyfills that users themselves added 🤔 but I guess if that's the case users need to make sure to add the polyfills before they add sentry (which they should have done already anyhow in order for Sentry not to break).

Copy link
Member

Choose a reason for hiding this comment

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

Whatever we do here will be better than what already exists, so I think this is fine. We are being good citizens via the in check on the prototype anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 then let's :shipit:

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

As discussed, I think it's okay for us to ship the simple polyfills for now. We can reevaluate if users report problems with that.

Also as discussed, we can disable the new ESLint rule on framework SDKs on a case-by-case basis so no need for this to happen now.

@mydea mydea merged commit 0e3552d into develop Mar 22, 2023
52 checks passed
@mydea mydea deleted the fn/es5-test branch March 22, 2023 15:53
billyvg added a commit that referenced this pull request Mar 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants