Only produce one build #1856
Only produce one build #1856
Conversation
|
Dang, why commitlint failing 🤔 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## v14 #1856 +/- ##
==========================================
+ Coverage 92.61% 92.68% +0.07%
==========================================
Files 148 148
Lines 2856 2856
Branches 776 776
==========================================
+ Hits 2645 2647 +2
+ Misses 195 192 -3
- Partials 16 17 +1 ☔ View full report in Codecov by Sentry. |
Supporting both cjs and esm is painful. esm is the future, but let's simplify the EDS build for now, and then update to esm at some point in the future.
Rollup already generates the type declarations.
Yeahhh I haven't gotten around to fixing this. I think commit messages with |
| "build:clean": "rm -rf lib/", | ||
| "build:declarations": "tsc --emitDeclarationOnly --project tsconfig.build.json", |
There was a problem hiding this comment.
Rollup is already producing type definitions, so this step is redundant. The types generated by each step are exactly the same.
| preserveModulesRoot: 'src', | ||
| sourcemap: true, | ||
| interop: 'auto', | ||
| }, |
There was a problem hiding this comment.
For posterity: there's an interesting thing here (including in the current EDS) where rollup is tree shaking out index.js files, but leaving the types for them...
So for example, the lib/Button directory ends up looking like this:
lib/Button
Button.js
Button.d.ts
index.d.ts <- There's an index.d.ts, but no corresponding index.js!
But, crucially, the import/export statements in the output are such that everything still works.
Basically, when doing import {Button} from 'eds'
- Node or bundlers load Button.js directly
- TypeScript thinks you're loading Button through index.js (which doesn't exist)
- But everything still works
Just an oddity I noticed and wanted to note.
We could stop that behavior by adding treeshake: false to the rollup config. Doing that results in everything lining up, but prints some warnings and I'm not sure if there are any ramifications or not.
There was a problem hiding this comment.
@ahuth we can create a ticket to disable tree-shaking and to fix the warnings, if that's the right direction to go for now? thoughts?
There was a problem hiding this comment.
Good idea. I'll create it.
There was a problem hiding this comment.
https://czi-tech.atlassian.net/browse/EFI-1643
Right now it's in the Medley epic. If there's somewhere better let me know.
Instead of producing 2 builds (cjs + esm), only produce one (cjs).
Why? Well, supporting dual builds is notoriously hard.
For my specific case, I'm working on making EDS support Vite in Remix
ESM is the future, and one day (like this year sometime) we should be able to use ESM in all our Remix apps and then change EDS to export esm modules directly.
TODO:
@chanzuckerberg/[email protected]