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

[v8] Drop polyfills from build output #8795

Open
mydea opened this issue Aug 11, 2023 · 3 comments
Open

[v8] Drop polyfills from build output #8795

mydea opened this issue Aug 11, 2023 · 3 comments
Milestone

Comments

@mydea
Copy link
Member

mydea commented Aug 11, 2023

Currently, we include a few polyfills in our build:

_asyncNullishCoalesce
_asyncOptionalChain
_asyncOptionalChainDelete
_createNamedExportFrom
_createStarExport
_interopDefault
_interopNamespace
_interopNamespaceDefaultOnly
_interopRequireDefault
_interopRequireWildcard
_nullishCoalesce
_optionalChain
_optionalChainDelete

These are a bit tricky, increase complexity and build size.

These can be split into two categories:

  1. nullish coalesce & optional chaining polyfills
  2. Bundler-related polyfills

The first category is actually disallowed by eslint rules, as the polyfills increase bundle size considerably when used. We do not use any nullish coalesce/optional chaining in all core/browser SDKs, only in node & nextjs.

I propose to remove these polyfills in v8, if we decide to bump the min. Node version to 14+. Node 14 supports both nullish coalescing as well as optional chaining.

I propose to leave the eslint rules for core/browser SDKs in place, but simply rely on them to ensure we don't use them outside of node context. We also have tests covering that the ES5 CDN bundles are valid ES5, which should cover any regressions there. This allows us to get rid of these polyfills:

_asyncNullishCoalesce
_asyncOptionalChain
_asyncOptionalChainDelete
_nullishCoalesce
_optionalChain
_optionalChainDelete

The other category of polyfills:

_createNamedExportFrom
_createStarExport
_interopDefault
_interopNamespace
_interopNamespaceDefaultOnly
_interopRequireDefault
_interopRequireWildcard

I propose we investigate if we can solve this with rollup instead.

Benefits of dropping the polyfills

  • Reduced complexity in our codebase & build
  • No need to have non-canonical exports (currently polyfills are imported directly from e.g. @sentry/utils/esm/buildPolyfills/index.js which is not ideal)
  • Reduced bundle size/increased performance for platforms that do support these (e.g. newer node) - can use native instead of polyfilled code
  • Browser/core packages already don't use these features, so no migration work necessary there
@mydea mydea added this to the 8.0.0 milestone Aug 11, 2023
@mydea
Copy link
Member Author

mydea commented Aug 18, 2023

Update for this issue: We dropped the build-related polyfills, so the remaining ones are the nullish coalesce & optional chaining ones.

@timfish
Copy link
Collaborator

timfish commented Mar 27, 2024

It looks like we can drop the polyfills entirely for @sentry/node and @sentry/profiling-node and @sentry/bun

@AbhiPrasad
Copy link
Member

So we did some refactors, but we still have https://github.com/getsentry/sentry-javascript/tree/develop/packages/utils/src/buildPolyfills left. Can we safely remove this?

@AbhiPrasad AbhiPrasad modified the milestones: 8.0.0, 9.0.0 Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants