-
-
Notifications
You must be signed in to change notification settings - Fork 227
Conversation
UMD is important as an easy way to use library via `<script>` tag. In this diff I added rollup with size snapshot plugin to track final size of the library with all dependencies (except react ones). I replaced mixed imports with `import * as React from 'react'` since they are being semantically incorrect are failed with rollup.
Thanks for the PR! Mixed imports are valid syntax, they are described in the Mozilla MDN: |
@FezVrasta I meant with react. It's mixed interop. You should use either default one (which do not include types) or namespace (which will be used when react land esm). |
Does the size-snapshot plugin create the minified bundle or do we have to add something like babili to generate it in a separated rollup config? |
@FezVrasta Nope it's only track the size. I will add min. |
rollup.config.js
Outdated
@@ -1,8 +1,16 @@ | |||
import nodeResolve from 'rollup-plugin-node-resolve'; | |||
import commonjs from 'rollup-plugin-commonjs'; | |||
import babel from 'rollup-plugin-babel'; | |||
import uglify from 'rollup-plugin-uglify'; |
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.
May we use rollup-plugin-babel-minify
instead? I got better outputs with it in Popper.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.
uglify 37.75 kb
babel-minify 38.15 kb
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.
You may need to tweak the default config a bit:
https://github.com/FezVrasta/popper.js/blob/master/packages/bundle/index.js#L18
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 did it. Nothing is changed.
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.
Cool thanks!
|
||
const input = './src/index.js'; | ||
|
||
const umdGlobals = { |
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.
Popper.js should be marked as external as well.
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.
Does it matter? Do you think somebody will use popper.js and react-popper in one script? For react-popper consumer there will be another action to add popper dependency.
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 want to get every week a new issue with someone that asks me to release a new UMD bundle with the new Popper.js version.
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.
Okay.
rollup.config.js
Outdated
{ | ||
input, | ||
output: { | ||
file: 'dist/index.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 probably be index.umd.min.js
to keep consistency
Should we add the UMD build to the |
@FezVrasta Nope. It's bad. webpack prefers |
@TrySound your changes are inconsistent with v0.10.3+ can you update the rollup config to be similar. This way migrating to 1.0.0 will be consistent/easy. Actually, I'll submit an updated PR. we have to fix the package.json in v10.3 so let me do that then also update in 1.0.x |
This is a question of your config. I don't think this package should be used other than with |
@virgofx what do you mean exactly? |
It looks like the babelrcs/rollups aren't consistent. Just would prefer to make those consistent. I was going to take v0.10.x and incorporate back into v1; however Try beat me however file paths output different, it's missing the remove-prop-types fixes, and production best-practice with NODE_ENV as all done in v0.10.x v.10.x
1.0
Plus update corresponding package.json scripts, I think v0.10.x is good now, we can cut v0.10.4 and that should be 100% ? If so, i'll make the corresponding changes now in 1.0 |
Note that in 1.x we are going to ship only UMD bundles, no CJS anymore |
That's totally fine, would prefer to drop the 'umd' sub folder then? or keep consistent? Plus was also going to incorporate readme updates |
Umh, maybe it's a good idea to keep it in case in the future we decide to add more bundles (maybe once ES modules get widespread in the browsers?). Good call. |
Yeah, it doesn't hurt. I'll make it all consistent later today. Just cut v0.10.4 -- Let's make sure that's 100%. Then I'll submit a PR later for 1.0 with the same testing and fixes for the SSR/ production builds , readme updates, package.json updates, and rollup sync so everything is smooth upgrading. |
I don't think there's anything missing in my config. |
Probably replace for warning. Don't know what it is. |
UMD is important as an easy way to use library via
<script>
tag.In this diff I added rollup with size snapshot plugin to track final
size of the library with all dependencies (except react ones).
I replaced mixed imports with
import * as React from 'react'
sincethey are being semantically incorrect are failed with rollup.