fix: Rework rollup config to output named exports (NODE-2712) #404
Conversation
Rollup's output was bundling the library to have one single default export this change makes the output preserve the named outputs. In addition the rollup config now compiles from typescript instead of from the typescript output.
src/bson.ts
Outdated
| EJSON | ||
| }; | ||
|
|
||
| export default bsonDefaultModuleExport; |
There was a problem hiding this comment.
Hmm, this looks pretty funky. Are we mixing named and default exports? It looks like you specified named in the rollup config, do you still need this here?
There was a problem hiding this comment.
Yes, because our original bundles had both named and default exports so we need to keep the behavior.
| @@ -60,7 +60,7 @@ export class ObjectId { | |||
| this.__id = id.__id; | |||
| } | |||
|
|
|||
| if (typeof id === 'object' && 'id' in id) { | |||
| if (typeof id === 'object' && id && 'id' in id) { | |||
There was a problem hiding this comment.
why is this change needed? the typeof check should prove the existence of the object (it's not typeof === undefined)
There was a problem hiding this comment.
typeof null is 'object', so this is checking for that
rollup.config.js
Outdated
| file: 'dist/bson.bundle.js', | ||
| format: 'iife', | ||
| name: 'BSON' | ||
| format: 'esm', |
There was a problem hiding this comment.
| format: 'esm', | |
| format: 'iife', |
There was a problem hiding this comment.
The format here is correct, babel is downgrading the code after rollup has done its transformations so babel is handling the bundle format. I use UMD in the babel preset because it is equivalent to an iife (babel doesn't have an iife output without a plugin, which seems unnecessary).
rollup.config.js
Outdated
| /* Browser UMD bundle */ | ||
| { | ||
| file: 'dist/bson.browser.umd.js', | ||
| format: 'umd', | ||
| name: 'BSON' | ||
| format: 'esm', |
There was a problem hiding this comment.
| format: 'esm', | |
| format: 'umd', |
There was a problem hiding this comment.
Same as comment above. This is correct for what we're doing here.
rollup.config.js
Outdated
|
|
||
| export default [ | ||
| { | ||
| input, | ||
| /* Browser ESM bundle */ |
There was a problem hiding this comment.
can you explain what's going on in this change? Also, it looks like the config is mostly duplicated below
There was a problem hiding this comment.
Yea the key difference is the babel settings. There is definitely a lot of duplication but after wrestling with the config for a better part of this project an explicit set of settings for each output is actually quite helpful rather than debugging a programatic config.
rollup.config.js
Outdated
| plugins: [nodeResolve({ preferBuiltins: true }), commonjs()] | ||
| plugins: [ | ||
| typescript({ tsconfig: './tsconfig.rollup.json' }), | ||
| nodeResolve({ preferBuiltins: true }), |
There was a problem hiding this comment.
I just noticed the comment above saying "note preferBuiltins false" which not actually the case here
There was a problem hiding this comment.
Oops yea out of date comment, good catch.
tsconfig.rollup.json
Outdated
| @@ -0,0 +1,32 @@ | |||
| { | |||
There was a problem hiding this comment.
does it make sense to embed this config directly in the rollup.config.js? I feel like otherwise it's just another file added to the heap of tooling files laying around. Is there a benefit to having this in its own file that I'm missing?
Rollup's output was bundling the library to have one single default export this change makes the output preserve the named outputs as well as have the default export. In addition the rollup config now compiles from typescript source instead of the typescript output.
NODE-2712