Switch the build process to rollup.js #130
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
realityking
force-pushed
the
rollup
branch
3 times, most recently
from
847ec8c
to
392d770
Compare
|
@aweary Could you have a look at this one? |
|
@aweary ping |
|
Can you please rebase and I'll take a look? |
|
@gaearon Rebased! And thanks for taking care of fbjs in prop-types as well :) |
|
While we're at it, could you also add Babel plugin for good measure? |
|
Done. Though I don't think it actually transforms anything 🤔 |
package.json
Outdated
| @@ -26,25 +26,24 @@ | |||
| "homepage": "https://facebook.github.io/react/", | |||
| "dependencies": { | |||
| "loose-envify": "^1.3.1", | |||
| "object-assign": "^4.1.1" | |||
| "object-assign": "^4.1.1", | |||
| "rollup-plugin-babel": "^3.0.4" | |||
rollup.config.js
Outdated
|
|
||
| export default [ | ||
| createConfig('prop-types.js'), | ||
| createConfig('prop-types.min.js', {uglify: true, production: true}) |
There was a problem hiding this comment.
Instead of configurable config factory it's cleaner to just write two configs and reuse input and output.name like here.
|
@TrySound Made the changes you suggested. Not sure if I like the new structure of the rollup config so I made that change in a different commit. |
rollup.config.js
Outdated
| babel({ | ||
| exclude: 'node_modules/**' | ||
| }), | ||
| replace(stripEnvVariables(options.production)), |
rollup.config.js
Outdated
| babel({ | ||
| exclude: 'node_modules/**' | ||
| }), | ||
| replace(stripEnvVariables(options.production)), |
|
Ups, fixed. (And more a bit easier to follow. |
|
@gaearon I think the PR is ready to merge. |
|
Hey @gaearon. Any thoughts on this? |
|
Hi @gaearon! Do you have a time to take a look at this? |
|
Due to bugs like rollup/rollup#2540, I don't think rollup is safe to use on any project at this time. Thanks for the PR! |
|
To be fair, we do use Rollup for React itself. Since prop-types doesn't do any feature detection I wouldn't worry about it. Ideally our build process should run tests on compiled bundles. That would be sufficient to feel good about it IMO since the surface area is very narrow. |
|
I‘m sorry, I‘m confused. As @Gaeron pointed out, rollup is widely used for React and related libraries. Why wouldn‘t it be a good a fit for prop-types? Rollup is not for every library and I wouldn‘t use it to bundle an application but it poses a clear win (size) for this lib. |
|
@gaearon fair enough! reopening. |
|
@gaearon Would be good to finally merge it. |
|
@ljharb volunteered to help clean up the repo so I trust him to make these decisions. |
Makes the minified UMD build 1.2kb smaller
|
I went through my open PR and found this gem from almost 3 (!) years ago. I've rebased it and upgraded all added dependencies to be the same version as in the react repository. I'd still love to see it merged. What do y'all think? As an aside, going to the newest version of the rollup dependencies would require upgrading babel to version 7. Happy to do that if there's interest in that. |
Makes the minified UMD build 1.2kb smaller. Rollup also eliminates a lot of closures, I suspect the code would be a tiny bit faster.