Publish bundles instead of modules, use babel only for transpilation #2214
Conversation
| '@typescript-eslint/consistent-type-assertions': [2, { assertionStyle: 'as', objectLiteralTypeAssertions: 'never' }], | ||
| '@typescript-eslint/consistent-type-definitions': [1, 'interface'], | ||
| '@typescript-eslint/consistent-type-imports': [1, { prefer: 'no-type-imports' }], | ||
| '@typescript-eslint/consistent-type-imports': 1, |
There was a problem hiding this comment.
I've tweaked this so we'll now use type imports, this is to help babel elide type imports, otherwise it cannot always know to remove some imports, especially in files like src/index.ts.
| "@babel/proposal-nullish-coalescing-operator", | ||
| "@babel/proposal-optional-chaining" |
There was a problem hiding this comment.
Fun fact: with our browserslist config, and our code, aside from these two transform plugins, @babel/preset-env does nothing.
| "@babel/proposal-optional-chaining" | ||
| ] | ||
| }], | ||
| ["@babel/react", { "useSpread": true }], |
There was a problem hiding this comment.
I'll change this to use the new runtime in #2184
| "@babel/typescript" | ||
| ], | ||
| "plugins": [ | ||
| ["@babel/transform-runtime", { "useESModules": true }], |
There was a problem hiding this comment.
We might have to add @babel/runtime in our dependencies in the future, but currently no helpers get imported in our bundles.
useESModules should also be set to false for the cjs bundle.
| 'react', | ||
| 'react-dom' | ||
| ], | ||
| external: id => !id.startsWith('.') && !isAbsolute(id), |
There was a problem hiding this comment.
This will mark all non-relative imports as external, in other words all npm dependencies.
| { | ||
| "presets": [ | ||
| ["@babel/env", { | ||
| "loose": true, |
There was a problem hiding this comment.
Should we be using loose?
There was a problem hiding this comment.
Original code:
ref.current?.focus({ preventScroll: true });With loose enabled:
(_ref$current = ref.current) == null ? void 0 : _ref$current.focus({With loose disabled:
(_ref$current = ref.current) === null || _ref$current === void 0 ? void 0 : _ref$current.focus({In non-loose mode, the value is explicitely checked for both null and undefined, as it needs to work with document.all, but we don't use document.all anywhere so it's safe.
It might matter in the future for new js syntax features, but loose will only break edge cases so I think it's fine.
| { | ||
| "extends": "./tsconfig.json", | ||
| "compilerOptions": { | ||
| "emitDeclarationOnly": false, |
There was a problem hiding this comment.
Do we need this if we have noEmit?
There was a problem hiding this comment.
Yes unfortunately.
> react-data-grid@7.0.0-canary.30 typecheck
> tsc -p tsconfig.all.json
tsconfig.all.json:4:5 - error TS5053: Option 'emitDeclarationOnly' cannot be specified with option 'noEmit'.
4 "noEmit": true
~~~~~~~~
Found 1 error.
rollup.config.mjs
Outdated
| nodeResolve() | ||
| babel({ | ||
| babelHelpers: 'runtime', | ||
| skipPreflightCheck: true, |
There was a problem hiding this comment.
Not sure if it is worth enabling it? Does it slow down the build by much?
There was a problem hiding this comment.
It builds in 1s in both cases, but I ran into issues before so I had to add this. I'll remove it now.
2 participants
Pros:
clsxcalls are optimized, users don't need to add an extra babel plugin.npmfootprint:We still use
tscto output declaration files.