Provide cjs and esm bundles with information about treeshakability #429
Conversation
There was a problem hiding this comment.
This is not my area of expertise. Can you please elaborate on the motivations behind this change and the benefits of it?
Thanks for seeking to improve our builds @TrySound 👏
| @@ -15,26 +15,22 @@ | |||
| "bugs": { | |||
| "url": "https://github.com/atlassian/react-beautiful-dnd/issues" | |||
| }, | |||
| "main": "lib/index.js", | |||
| "module": "esm/index.js", | |||
| "main": "dist/react-beautiful-dnd.cjs.js", | |||
There was a problem hiding this comment.
is it common to do this rather than use a seperate folder?
There was a problem hiding this comment.
Dumb question, but cjs = CommonJS? 😊
There was a problem hiding this comment.
I think it is not necessary to create a folder for only one file in it. We have five file in the end which look very nice in one folder.
Yes, esm and cjs are the most popular shorthands.
| "build:dist": "rollup -c", | ||
| "build:flow": "echo \"// @flow\n\nexport * from '../src';\" | tee lib/index.js.flow esm/index.js.flow", | ||
| "build:flow": "echo \"// @flow\n\nexport * from '../src';\" > dist/react-beautiful-dnd.cjs.js.flow", |
There was a problem hiding this comment.
Is flow okay with this? Do we want it also for the .esm.js file too?
There was a problem hiding this comment.
I don't think it's necessary. Users usually do not specify esm entry directly, but rely on bundler.
rollup.config.js
Outdated
| @@ -47,6 +51,36 @@ const getUMDConfig = ({ env, file }) => { | |||
| }; | |||
|
|
|||
| export default [ | |||
| getUMDConfig({ env: 'development', file: 'dist/react-beautiful-dnd.js' }), | |||
| getUMDConfig({ env: 'development', file: 'dist/react-beautiful-dnd.umd.js' }), | |||
|
|
|||
| getUMDConfig({ env: 'production', file: 'dist/react-beautiful-dnd.min.js' }), | |||
There was a problem hiding this comment.
This will break pattern :(
react-beautiful-dnd.cjs.js
react-beautiful-dnd.cjs.js.flow
react-beautiful-dnd.esm.js
react-beautiful-dnd.min.js
react-beautiful-dnd.umd.js
I just would like to see this pattern widespread, so min in most cases could mean umd format. I already used it in a set of modules.
|
There are a few benefits
|
|
Can you please elaborate on:
|
|
Users may use some internals with |
|
Perfect. Thanks |
rollup.config.js
Outdated
| const input = './src/index.js'; | ||
|
|
||
| const extensions = ['.js', '.jsx']; | ||
|
|
||
| const isExternal = id => !id.startsWith('.') && !id.startsWith('/'); |
There was a problem hiding this comment.
can you please add a comment above this line in the code explaining its purpose? Other than that I think I am good to go!
rollup.config.js
Outdated
| plugins: [ | ||
| resolve({ extensions }), | ||
| babel(getBabelOptions()), | ||
| strip({ debugger: true }), |
There was a problem hiding this comment.
I am keen to keep console.* statements for the non-minified builds. Can the strip function be updated so that only the minified build strips console.*? https://github.com/rollup/rollup-plugin-strip#usage
There was a problem hiding this comment.
From non minified umd too? It seems console was always stripped from both umd bundles
|
Yep. However the esm and cjs (/lib) builds previously had console.
…On Tue, 10 Apr 2018 at 6:11 pm, Bogdan Chadkin ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In rollup.config.js
<#429 (comment)>
:
> + babel(getBabelOptions()),
+ strip({ debugger: true }),
+ ],
+ },
+
+ {
+ input,
+ output: {
+ file: pkg.module,
+ format: 'es',
+ },
+ external: isExternal,
+ plugins: [
+ resolve({ extensions }),
+ babel(getBabelOptions()),
+ strip({ debugger: true }),
From non minified umd too? It seems console was always stripped from both
umd bundles
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#429 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACFN7W30-cnfaZaIr4iwp7Id_N1K3l_Oks5tnGkzgaJpZM4TNRv4>
.
|
|
Done |
|
Well done @TrySound. Thanks for your efforts in getting this in! |
Ref #405
Closes #428