-
Notifications
You must be signed in to change notification settings - Fork 4
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(rollup): mark dependencies as external #1558
Conversation
Codecov Report
@@ Coverage Diff @@
## ah-ssr-build #1558 +/- ##
=============================================
Coverage 91.84% 91.84%
=============================================
Files 279 279
Lines 4278 4278
Branches 789 789
=============================================
Hits 3929 3929
Misses 324 324
Partials 25 25 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Can't figure out why some of the UI Tests have diffs. I swear they look the same :/ |
aca0c16
to
5e773df
Compare
I think something in Chromatic changed since even pr's with unrelated changes had the same visual diffs across all the stories, so accepted in that PR. Rebased and the diffs are gone |
plugins: [ | ||
nodeResolve(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What problem were you seeing? Node modules were included in the rollup build?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. Wasn't clear before, but I think I got it:
- By default rollup only includes relative modules in the bundle
- The node-resolve plugin lets us include node_modules in the bundle
With this new config, is React being included in the bundle?
If so, should that be explicitly marked as external (instead of included in the bundle)? We may want any peer deps to be "external".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nvm, Jin and I looked into this more and all's good with it.
The plugin just tells rollup how to resolve paths to node_modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment explaining what the external option does
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🤘
* build: use rollup to build JS and styles - To better support server-side rendering - By producing a single .css file - Instead of delegating css module processing to consuming apps * refactor(rollup): remove spritemap and commonjs from config * build: remove styles build from main build command * refactor: import css variables into index * build(rollup): mark dependencies as external (#1558) * build(rollup): mark dependencies as external * build(rollup): use node resolve plugin * docs(build): explain rollup external option * build!: generate cjs and type declarations (#1572) * build: generate cjs and esm, separate types build * build: add sourcemaps and add declaration back to tsconfig * build: cjs and js in the same output * build: add conditional exports * build: remove build types script * refactor: revert tw config to ts * build: bump target to es2018 and grep for tslib * chore(release): 12.0.0-alpha.0 * refactor: dry css exports * build!: remove some tokens from build (#1581) * refactor: remove references to unexported files * build!: no longer include some tokens in build * refactor: remove unused commonjs plugin (#1582) * chore(release): 12.0.0-alpha.1 * refactor: use named import for clsx * chore: remove rollup plugin commonjs * Revert "refactor: use named import for clsx" This reverts commit d58182b. * docs: explicate tslib grep * fix: no need for ts expect error --------- Co-authored-by: Andrew Huth <[email protected]>
## [12.0.0](v11.1.1...v12.0.0) (2023-04-17) ### ⚠ BREAKING CHANGES * use rollup (#1555) ### Features * export some subcomponents ([#1579](#1579)) ([2857ae4](2857ae4)) * **TextareaField:** add character length counter ([#1580](#1580)) ([ff6226f](ff6226f)) ### Bug Fixes * restore check for undefined any types ([#1585](#1585)) ([c7fae07](c7fae07)) * **Skeleton:** mark .Rect as deprecated ([#1586](#1586)) ([405f81b](405f81b)) * sync typography presets to documentation ([#1592](#1592)) ([b56eadb](b56eadb)) * **typography:** add missing eds-font-size-20 ([#1591](#1591)) ([de5dd03](de5dd03)) ### build * use rollup ([#1555](#1555)) ([d794696](d794696)), closes [#1558](#1558) [#1572](#1572) [#1581](#1581) [#1582](#1582)
EDS-884
Summary:
@rollup/plugin-node-resolve
as listed in rollup docs to mark external deps as externalpackage.json
is good practice (never seen it before lol)Test Plan:
yarn build:js