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

Update plugin rollup polyfill package #3621

Merged
merged 4 commits into from
Apr 6, 2023
Merged

Conversation

garrettjstevens
Copy link
Collaborator

This replaces rollup-plugin-node-builtins and rollup-plugin-node-globals with rollup-plugin-polyfill-node. It's a drop-in replacement, and plugin authors won't have to change anything in their code.

The reason for this update is that the prior packages were not maintained, and when trying to bundle code into a plugin that used newer JS syntax, (such as optional chaining), the prior packages could not handle it.

I tried to also see if there was a way to limit which node polyfills were scanned for and bundled, but neither the prior or new plugins provide a way to do that. As it stands, though, the polyfills are only included if their use is detected, so it won't needlessly increase the bundle size.

This also adds react/jsx-runtime to the ReExports list. It's not something that's ever imported directly by the user, but if their code uses the JSX transform (e.g. their tsconfig.json uses "jsx": "react-jsx"), this allows them to reduce their bundle size by relying on the package that JBrowse has already loaded.

@garrettjstevens garrettjstevens self-assigned this Apr 3, 2023
@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Apr 3, 2023
@codecov
Copy link

codecov bot commented Apr 3, 2023

Codecov Report

Merging #3621 (2c05640) into main (9fe2420) will increase coverage by 0.00%.
The diff coverage is n/a.

❗ Current head 2c05640 differs from pull request most recent head f21a68d. Consider uploading reports for the commit f21a68d to get more accurate results

@@           Coverage Diff           @@
##             main    #3621   +/-   ##
=======================================
  Coverage   62.96%   62.97%           
=======================================
  Files         871      871           
  Lines       30193    30193           
  Branches     7271     7271           
=======================================
+ Hits        19012    19014    +2     
+ Misses      10996    10994    -2     
  Partials      185      185           
Impacted Files Coverage Δ
packages/core/ReExports/modules.tsx 11.16% <ø> (ø)

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cmdcolin
Copy link
Collaborator

cmdcolin commented Apr 4, 2023

the jsx transform one is interesting, may fix GMOD/jbrowse-plugin-template#20

I would say the alternative to this approach is making the user be more 'explicit' in what is polyfilled, but this may be ok. we could even provide e.g. a commented out section of the rollup config showing the 'explicit' method of polyfilling perhaps, but that doesn't need to block this

@cmdcolin cmdcolin requested review from cmdcolin and removed request for cmdcolin April 5, 2023 01:17
@cmdcolin cmdcolin added bug Something isn't working housekeeping needs to be tidied up, is a drag on team performance and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels Apr 6, 2023
@cmdcolin cmdcolin merged commit 9a9978b into main Apr 6, 2023
@cmdcolin cmdcolin deleted the use_rollup-plugin-polyfill-node branch April 6, 2023 02:45
cmdcolin pushed a commit that referenced this pull request May 18, 2023
* Replace node polyfill rollup plugins

* Re-export 'react/jsx-runtime'

* Comment on usage of include:null in nodePolyfill

* Fix typos
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working housekeeping needs to be tidied up, is a drag on team performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants