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

deps: use rollup-plugin-node-externals #83

Merged
merged 1 commit into from
Jun 11, 2022

Conversation

agilgur5
Copy link
Owner

Summary

Use rollup-plugin-node-externals instead of custom external code

Details

@agilgur5 agilgur5 added the kind: internal Changes only affect the internals and not the API or usage label Jun 11, 2022
@agilgur5 agilgur5 force-pushed the deps-rollup-plugin-node-externals branch from 63fb3f7 to 0b00c7e Compare June 11, 2022 02:55
@agilgur5
Copy link
Owner Author

Travis CI just didn't run for whatever reason so I finally bit the bullet and migrated to GH Actions in #84. Rebased on top of that now

@codecov
Copy link

codecov bot commented Jun 11, 2022

Codecov Report

Merging #83 (6ed1a63) into main (6ed1a63) will not change coverage.
The diff coverage is n/a.

❗ Current head 6ed1a63 differs from pull request most recent head ce2411a. Consider uploading reports for the commit ce2411a to get more accurate results

@@            Coverage Diff            @@
##              main       #83   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines           75        75           
  Branches         9         9           
=========================================
  Hits            75        75           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6ed1a63...ce2411a. Read the comment docs.

- instead of the custom JS code I wrote
  - I was thinking of separating that into a plugin, but someone already
    did that!
    - and, importantly, also covered submodules, unlike _most_ of the
      externals plugins
      - see regex here: https://github.com/Septh/rollup-plugin-node-externals/blob/11a7b4454f57c76436e71ecead0cc59ab0cc3b80/src/index.ts#L106

- put it _before_ `node-resolve` as the docs state

- reduces my code a bit, which is always nice, and will make it easier
  to split my Rollup config into its own "preset" package later on
@agilgur5 agilgur5 force-pushed the deps-rollup-plugin-node-externals branch from 0b00c7e to ce2411a Compare June 11, 2022 03:00
@agilgur5 agilgur5 added the scope: dependencies Pull requests that update a dependency file label Jun 11, 2022
Copy link
Owner Author

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

LGTM, yay for simpler code!

I did also confirm locally that this produces an equivalent bundle ✅

@agilgur5 agilgur5 merged commit 0154531 into main Jun 11, 2022
@agilgur5 agilgur5 deleted the deps-rollup-plugin-node-externals branch June 11, 2022 03:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: internal Changes only affect the internals and not the API or usage scope: dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant