-
Notifications
You must be signed in to change notification settings - Fork 24
Added UMD compiled version to build process #26
base: master
Are you sure you want to change the base?
Conversation
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.
Hi @dhlavaty thanks a lot for your PR 👍 I've added some comments here, could you please review? Thanks!
@@ -4,12 +4,13 @@ | |||
"description": "Denormalizer for normalizr", | |||
"main": "lib/index.js", | |||
"files": [ | |||
"dist", | |||
"lib" |
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.
We can use only one built version 😊 Remove lib
and set "main": "dist/index.js"
above.
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.
Why one version ? Build process is preparing two different versions on purpose.
This is setup exactly as for example normalizr
(or other node libraries). Your current version (only transpiled from ES6 to ES5) will be in lib
as always. So library users will include it, in their npm projects like always. They can compile their own bundle, minify, ...
But dist
will contain special UMD version of the library, that can be used directly in a browser - for example allowing it in JSBin playground via https://unpkg.com/. Thats why the file in dist
is denormalizer.min.js
and not index.js
.
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.
Why one version
An UMD build works for CommonJS, AMD and global var, so I wouldn't mind to include this dist even for the standard lib. Anyway if this is the common practice then i can live with it 😄
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.
It is quite common. Check React
, Redux
or normalizr
source codes.
"lib" | ||
], | ||
"scripts": { | ||
"cover": "babel-node ./node_modules/istanbul/lib/cli cover -- _mocha --recursive --reporter spec", | ||
"prebuild": "rimraf dist lib", | ||
"build": "babel src --out-dir lib", | ||
"build": "webpack && babel src --out-dir lib", |
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.
We can build everything with webpack, i.e. skip the babel
command here...
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.
Don't think so. See ma answer #26 (comment)
"babel-preset-es2015": "^6.16.0", | ||
"babel-preset-stage-1": "^6.16.0", |
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.
I don't think we need stage-1 at all – is there a reason of why you added it?
] | ||
}, | ||
output: { | ||
filename: 'dist/denormalizr.min.js', |
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.
This should be named index.js
, not denormalizer.min.js
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.
See my answer #26 (comment)
'process.env': { | ||
'NODE_ENV': JSON.stringify('production') | ||
} | ||
}), |
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.
I don't think OccurenceOrderPlugin
and setting the NODE_ENV to production
is required – or am I missing something? 🤔
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.
My answer #26 (comment)
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.
The comment doesn't answer the question :) I'm curious why do we need OccurenceOrderPlugin
and the NODE_ENV set to production
here. OccurenceOrderPlugin
ok, no much benefits using it, however the source doesn't include any particular conditions for production
.
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.
We want to have precompiled production and development UMD builds in the dist
folder. You are probably right with OccurenceOrderPlugin
, I was just copying webpack.config from different very simple setup. Do you want me to remove it ?
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.
Yeah I'd remove both the plugins, thanks!
test: /\.js$/, | ||
loader: 'babel', | ||
exclude: /node_modules/, | ||
query: { |
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.
@dhlavaty babel-loader uses .babelrc
when available – it should be safe to remove the query
object here, so we don't need to update two files when the babel config changes.
PR for #25
Running
npm run build
will also create an UMD version indist
sub dir.