build: refactored build process #109

FezVrasta · 2018-03-23T15:13:38Z

fixes #106, fixes #80, fixes #82

This PR:

replaces webpack with Rollup for production builds
replaces webpack with parcel for demo/example builds
adds the wrap mode of babel-plugin-transform-react-remove-prop-types to allow to strip PropTypes.
properly marks all the needed dependencies as external, reducing the bundle size and allowing consumers to specify their own versions if needed.

Todo:

add minifier
add names for UDM dependencies

I'd love to have some feedbacks and have few people test the new output to make sure it still works as expected.

You are able to test this branch using react-popper@next

TheSharpieOne · 2018-03-23T18:24:53Z

package.json

+    "build:cjs-min":
+      "MINIFY=true rollup -c --output.format cjs --output.name 'react-popper' --output.file dist/react-popper.min.js",
+    "demo": "parcel --out-dir demo/dist demo/index.html",
+    "prepare": "npm run build",


Correct me if I am wrong, but perpare will run when users install this library as a dependency (as well as before the package is published). Did you intend to have it build every time it is installed?
prepublishOnly would run the build only prior to publishing (not on install).

npm sucks, there's not a real solution to this...

If you use prepare, it will run on both the cases you mentioned, but if you use prepublishOnly it's not going to wait for it to finish before publishing, at least that was the case the last time I checked the discussion on the npm repo.

This is the gist of the discussion:

but you can't (currently) put code into prepublishOnly that allows you to run a build step before publishing

npm/npm#15454 (comment)

I agree,

Since npm@1.1.71, the npm CLI has run the prepublish script for both npm publish and npm install, because it's a convenient way to prepare a package for use. It has also turned out to be, in practice, very confusing. As of npm@4.0.0, a new event has been introduced, prepare, that preserves this existing behavior. A new event, prepublishOnly has been added as a transitional strategy to allow users to avoid the confusing behavior of existing npm versions and only run on npm publish (for instance, running the tests one last time to ensure they're in good shape).

Justkant · 2018-03-24T10:05:53Z

src/index.js

+export { default as Manager } from './Manager';
+export { default as Target } from './Target';
+export { default as Popper } from './Popper';
+export { default as Arrow } from './Arrow';


I hope this will not cause the same problem that #93 solved

Looking at the compiled code everything looks good, anyway the old syntax is not valid at all with Rollup so there's not much choice.

#93 was fixing a bug with the babel output not the webpack build.

You could compare the file lib/react-popper.js to old ones to be sure:
react-popper.js@v0.8.0 <-- Seems good for everyone
react-popper.js@v0.8.1 <-- Wrong ESM
react-popper.js@v0.8.2 <-- Wrong This was when I used the same syntax as you here.
react-popper.js@v0.8.3 <-- Seems good for everyone

FezVrasta · 2018-03-27T13:31:55Z

@Justkant @TheSharpieOne did anyone of you have a chance to test this? I ran it on some internal code and it seems to work fine, but I'd like you to double check if possible

Justkant · 2018-03-27T14:12:41Z

Hi, I had no time to test it so far.
I can give it a try tonight ✌️

TheSharpieOne · 2018-03-27T17:17:51Z

I just tried it with reactstrap as that's what was giving me issues before. It is not working and is acting similar to 0.8.2

> rollup -c

🚨   Error: 'Manager' is not exported by node_modules/react-popper/lib/react-popper.js
https://github.com/rollup/rollup/wiki/Troubleshooting#name-is-not-exported-by-module
src/Dropdown.js (7:9)
5: import PropTypes from 'prop-types';
6: import ReactDOM from 'react-dom';
7: import { Manager } from 'react-popper';
            ^
8: import classNames from 'classnames';
9: import { mapToCssModules, omit, keyCodes, deprecated } from './utils';

I can use the link in the error to determine that I can add additional configurations to my rollup config (telling it about react-popper's exports), but I don't think that should be needed.

0.9.0: https://unpkg.com/react-popper@0.9.0/lib/react-popper.js is the same as
0.8.2: https://unpkg.com/react-popper@0.8.2/lib/react-popper.js which had the same issue
0.8.3: https://unpkg.com/react-popper@0.8.3/lib/react-popper.js works.

FezVrasta · 2018-03-27T18:45:01Z

Umh lib should preserve the import/export.. something is wrong with the build

Justkant · 2018-03-28T10:28:52Z

.babelrc

-    "transform-export-extensions"
-  ]
+  "presets": [["env", { "modules": false }], "stage-2", "react"],
+  "plugins": ["transform-class-properties"]


Here you don't need to use the stage-2
Don't you still need transform-object-rest-spread ?

Also I used transform-export-extensions without { "modules": false } to be able to transform export Manager from './Manager' correctly. But it's a problem with rollup if you want to use his native support of esm.
We could use babel env feature to use rollup native modules & let babel transform for the lib build

@FezVrasta
Do we need everything in stage-2 & stage-3 ?
I think we only need transform-class-properties and transform-object-rest-spread, the babel-preset-env takes care of the rest.
Anyway it's ok to use stage-2 but transform-class-properties becomes redundant here

I kept stage-2 and removed the plugin

Justkant · 2018-03-28T10:33:30Z

The part that I don't understand is why there's a problem with this version react-popper.js@v0.8.2
Maybe missing explicit exports.Something at the end to be correctly parse, like react-popper.js@v0.8.3

Justkant · 2018-03-28T10:39:16Z

@FezVrasta do you really think that lib should be ES6 import/exports ?
Last time I did that, it broke for some people.

Also tried react-popper@next without any problem on my end.

FezVrasta · 2018-03-28T10:59:37Z

the module target is supposed to use the ES modules, so import/export 🤷‍♂️

Justkant · 2018-03-28T11:14:43Z

You're right, I didn't see the module target for ESM. Prior to this PR dist was only containing umd builds right ?
Also you didn't delete the webpack.prod.config.js

rolandjitsu · 2018-03-28T12:38:08Z

@FezVrasta don't forget to copy the TS definitions file into dist when bundling (or when releasing) as it's missing right now in the npm dist for next tag (but still referenced from 'package.json').

FezVrasta · 2018-03-28T14:32:42Z

@rolandjitsu is my last commit okay?

TheSharpieOne · 2018-03-28T14:59:22Z

FWIW, I think the lib/ should contain the individual cjs files. The dist should contain bundled files for cjs, esm, and umd.

Reasoning:
People may want to include individual files/components. lib would allow them to do that with cjs. esm supports tree-shaking, so even if their is only the one file, the unused exports will be removed via tree-shaking. With tree-shaking, there isn't really a point to have individual files for esm. The bundle cjs file helps avoid some overhead with the helper functions being in every file (for instance, see https://unpkg.com/react-popper@0.9.0/lib/Target.js and https://unpkg.com/react-popper@0.9.0/lib/Popper.js and notice they both include the same helpers).

TL;DR: making lib be cjs rather than esm and putting a single esm file in dist provides the ability to import single components in both cjs and esm (the latter via tree-shaking) and more efficient full bundles for both cjs and esm.

FezVrasta · 2018-03-28T15:13:48Z

lib is supposed to use ES Modules, not CommonJS AFAIK

FezVrasta · 2018-03-28T15:30:21Z

Please give a spin to react-popper@0.9.1-alpha.1/react-popper@next. The /lib build should be okay now.

TheSharpieOne · 2018-03-28T16:42:36Z

0.9.1-alpha.1 works both for building other libraries with rollup (such as reactstrap itself) and when consuming it in an end project with webpack (such as when using reactstrap in your project). Those are pretty much my tests to ensure it doesn't break reactstrap.

I just wanted to note this (food for thought):
The individual transpiled components are great to cherry pick what you need into your project; not including everything (though again, a single esm with tree-shaking accomplishes the same thing). Currently, the only way to cherry pick would be with esm (which wouldn't be needed since it already does tree-shaking). You cannot cherry pick with cjs as other projects allow (check out projects from reactjs). However because they are individually transpiled, each of them will contain the Babel helpers which adds up quickly when importing multiple components. For example the transpiled Manager component consists for almost 50% of Babel helpers. While minification and gzip (over the wire) can help the size, it still is larger than providing a single esm file.

rolandjitsu · 2018-03-29T07:09:03Z

@FezVrasta it's perfect.

fixes #106, #80, #82

FezVrasta · 2018-03-30T12:12:03Z

@TheSharpieOne you're right about the file size, but atm we are shipping the whole popper.js library so I think it's still an improvement :-) If we can come with a solution to this last size problem we can fix it later.

virgofx · 2018-03-30T13:28:28Z

@FezVrasta Just to confirm it looks like the unminified filesize went from 103KB (0.8.3) to 15KB (0.9.1) with similar savings on the minified side (29.4KB -> 7.4KB) !?!?! This is amazing!

FezVrasta · 2018-03-30T13:31:35Z

Yes that's right. Previously react-popper bundled its own version of Popper.js, now it references it as external dependency.

I'm also working on a React 16 rewrite that should reduce even more the final size and simplify code and API.

virgofx · 2018-03-30T13:33:55Z

Gotcha. Not a problem so at least the prop-types stuff got included (that should help a little in the production builds). With 0.9.1+ will end users now need to include both react-popper and popper.js? Or just react-popper with end users configuring whether or not popper gets included via their applications externals setting.

FezVrasta · 2018-03-30T13:35:40Z

react-popper defines popper.js as dependency, so it will get automatically installed and included.

The advantage is that if you already use popper.js on other parts of the code, the bundler will be able to share the same package instead of duplicating it.

virgofx · 2018-03-30T13:36:02Z

Awesome, gotcha.

Justkant · 2018-03-30T13:43:53Z

Nice to see all those improvements, nice work @FezVrasta ✌️
Can't wait to see the rewrite with the new Context API & Fragments or other things that you have in mind 😄

FezVrasta · 2018-03-30T13:45:40Z

It's going to be render props powered, context managed by the new context API, and not much else actually.

The API will be much leaner and the internal logic will get simplified.

TheSharpieOne · 2018-03-30T14:42:25Z

Are you going to target 16.3.0 as the min version of react for the new context API?

FezVrasta · 2018-03-30T14:44:04Z

I'm not sure what version I do specifically need, I think I just need fragments support actually? But probably it could even work on React 15 if one doesn't use the renderProps in some weird manner.

The API looks like this so far:

<Manager>
  <Target>{getTargetRef =>
    <button ref={getTargetRef}>target</button>
  }</Target>
  <Popper>{({ getPopperRef, style }) =>
    <div ref={getPopperRef} style={style}>
      popper
      <Arrow>{({ getArrowRef, style }) =>
        <div ref={getArrowRef} style={style} />
      }</Arrow>
    </div>
  }</Popper>
</Manager>

FezVrasta force-pushed the popperjs-external branch 2 times, most recently from 645ccaf to 67f522c Compare March 23, 2018 15:57

FezVrasta mentioned this pull request Mar 23, 2018

react-popper undefined when bundled by rollup as an external and used in webpack app #66

Closed

FezVrasta force-pushed the popperjs-external branch from 67f522c to 1be2848 Compare March 23, 2018 16:05

FezVrasta mentioned this pull request Mar 23, 2018

fix: minified umd build without prop-types #94

Closed

TheSharpieOne reviewed Mar 23, 2018

View reviewed changes

FezVrasta force-pushed the popperjs-external branch from 1be2848 to e2a672b Compare March 23, 2018 18:35

Justkant reviewed Mar 24, 2018

View reviewed changes

Justkant reviewed Mar 28, 2018

View reviewed changes

Federico Zivolo added 4 commits March 30, 2018 14:09

build: refactored build process

730cfcb

fixes #106, #80, #82

build(fix): include TS types

46684af

build: More fixes

831f986

v0.9.1-alpha.1

31aafea

FezVrasta force-pushed the popperjs-external branch from 27641dd to 31aafea Compare March 30, 2018 12:10

FezVrasta merged commit 9a62dc4 into master Mar 30, 2018

FezVrasta deleted the popperjs-external branch March 30, 2018 13:31

TheSharpieOne mentioned this pull request Apr 5, 2018

Feature Request: Clarify package contents & allow for better tree shaking #127

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: refactored build process #109

build: refactored build process #109

FezVrasta commented Mar 23, 2018 •

edited

TheSharpieOne Mar 23, 2018

FezVrasta Mar 23, 2018 •

edited

TheSharpieOne Mar 23, 2018

Justkant Mar 24, 2018

FezVrasta Mar 24, 2018

Justkant Mar 27, 2018

FezVrasta commented Mar 27, 2018

Justkant commented Mar 27, 2018

TheSharpieOne commented Mar 27, 2018

FezVrasta commented Mar 27, 2018

Justkant Mar 28, 2018

Justkant Mar 28, 2018

FezVrasta Mar 28, 2018

Justkant commented Mar 28, 2018

Justkant commented Mar 28, 2018 •

edited

FezVrasta commented Mar 28, 2018

Justkant commented Mar 28, 2018

rolandjitsu commented Mar 28, 2018 •

edited

FezVrasta commented Mar 28, 2018

TheSharpieOne commented Mar 28, 2018

FezVrasta commented Mar 28, 2018

FezVrasta commented Mar 28, 2018 •

edited

TheSharpieOne commented Mar 28, 2018

rolandjitsu commented Mar 29, 2018

FezVrasta commented Mar 30, 2018

virgofx commented Mar 30, 2018 •

edited

FezVrasta commented Mar 30, 2018 •

edited

virgofx commented Mar 30, 2018 •

edited

FezVrasta commented Mar 30, 2018

virgofx commented Mar 30, 2018

Justkant commented Mar 30, 2018

FezVrasta commented Mar 30, 2018

TheSharpieOne commented Mar 30, 2018 •

edited

FezVrasta commented Mar 30, 2018 •

edited

build: refactored build process #109

build: refactored build process #109

Conversation

FezVrasta commented Mar 23, 2018 • edited

Choose a reason for hiding this comment

FezVrasta Mar 23, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FezVrasta commented Mar 27, 2018

Justkant commented Mar 27, 2018

TheSharpieOne commented Mar 27, 2018

FezVrasta commented Mar 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Justkant commented Mar 28, 2018

Justkant commented Mar 28, 2018 • edited

FezVrasta commented Mar 28, 2018

Justkant commented Mar 28, 2018

rolandjitsu commented Mar 28, 2018 • edited

FezVrasta commented Mar 28, 2018

TheSharpieOne commented Mar 28, 2018

FezVrasta commented Mar 28, 2018

FezVrasta commented Mar 28, 2018 • edited

TheSharpieOne commented Mar 28, 2018

rolandjitsu commented Mar 29, 2018

FezVrasta commented Mar 30, 2018

virgofx commented Mar 30, 2018 • edited

FezVrasta commented Mar 30, 2018 • edited

virgofx commented Mar 30, 2018 • edited

FezVrasta commented Mar 30, 2018

virgofx commented Mar 30, 2018

Justkant commented Mar 30, 2018

FezVrasta commented Mar 30, 2018

TheSharpieOne commented Mar 30, 2018 • edited

FezVrasta commented Mar 30, 2018 • edited

FezVrasta commented Mar 23, 2018 •

edited

FezVrasta Mar 23, 2018 •

edited

Justkant commented Mar 28, 2018 •

edited

rolandjitsu commented Mar 28, 2018 •

edited

FezVrasta commented Mar 28, 2018 •

edited

virgofx commented Mar 30, 2018 •

edited

FezVrasta commented Mar 30, 2018 •

edited

virgofx commented Mar 30, 2018 •

edited

TheSharpieOne commented Mar 30, 2018 •

edited

FezVrasta commented Mar 30, 2018 •

edited