fix: v0.x dist builds to ensure SSR works properly and popper.js is included in the dist builds #146

virgofx · 2018-04-21T19:00:25Z

Fixes #145

After 0.8.3, the new dist build using Rollup config failed to include popper.js and missed some optimization settings for ensuring SSR rendering works correctly.

Updated:

SSR rendering works using UMD build.
Ensure /dist builds contain popper.js
Removal of prop-types doesn't use wrap mode which would leave an empty {}. Instead, completely removed saving production builds a few bytes. Note that because the use of old contextTypes, prop-types will still be required until new Context 16.3 is used. At that point, production builds could completely reduce the dependency for prop-types.

@FezVrasta Can I create a separate PR that re-includes these rollup changes in the master branch such that /dist builds can continue to work once v1 becomes stable. Currently, the dist build was removed in the latest and will prevent 3rd party libraries that want to include popper capability via react-popper statically. Additionally, since it looks like work is being done to update context, the dist dependency on prop-types will go away 100%. I can also update the readme to explain the important/how to use for 3rd parties referencing statically as we do in Reactstrap.

/cc @TheSharpieOne Note pending successful merge + release, we need to update the readme in the next release to point to .../0.10.2/react-popper.umd.min.js (Note the umd portion) and update the default codepen JS references examples to all have the latest as well.

…ncluded in the dist builds.

FezVrasta · 2018-04-22T05:42:54Z

rollup.config.js

  ].filter(Boolean),
-  external: ['react', 'react-dom', 'prop-types', 'popper.js'],
+  external: ['react', 'react-dom', 'prop-types'],


We specifically want to leave popper.js out the build so that people can use their own version, mock it and reduce the final bundle size.

Why do you think it should be bundled?

Because that's how Reactstrap and various other 3rd party libraries are using react-popper in a static sense. (The removal of popper.js after 0.8.3 broke it for us essentially locking it to that version) Essentially, think about a lot of frameworks and applications that are using React ... they want to add drop-in tooltip support and not worry about having to include an underlying tooltip api separately.

As a compromise, I wouldn't mind creating 2 versions. That's actually what we do in Reactstrap as well ... we have a full.js version. This way people could include statically without popper.js to use their own tooltip API implementation or use a full drop-in tooltip react-popper implementation with popper.js.

Let me know if you're okay with that and I'll update the PR including some notes in the Readme. I'll just leave the current versions as they were and then add the corresponding full versions that get built for each cjs, umd version with a full suffix. That work?

One more thought ... I'd suggest updating the current cjs versions to include a cjs suffix. It's somewhat misleading as most people use umd as the default version imported statically (reference: React's builds and readmes). Adding a cjs suffix would help with that clarification.

virgofx · 2018-04-22T15:25:59Z

Not sure how to fix the travis-ci Rake issues (they're non-related to my changes).

Updated ./dist/ directory output with latest changes:

 14K Apr 22 15:02 react-popper.cjs.js
 17K Apr 22 15:02 react-popper.cjs.js.map
7.4K Apr 22 15:03 react-popper.cjs.min.js
 15K Apr 22 15:03 react-popper.cjs.min.js.map
 96K Apr 22 15:02 react-popper.full.cjs.js
145K Apr 22 15:02 react-popper.full.cjs.js.map
 31K Apr 22 15:03 react-popper.full.cjs.min.js
130K Apr 22 15:03 react-popper.full.cjs.min.js.map
102K Apr 22 15:02 react-popper.full.umd.js
154K Apr 22 15:02 react-popper.full.umd.js.map
 27K Apr 22 15:03 react-popper.full.umd.min.js
129K Apr 22 15:03 react-popper.full.umd.min.js.map
 16K Apr 22 15:02 react-popper.umd.js
 19K Apr 22 15:02 react-popper.umd.js.map
6.9K Apr 22 15:03 react-popper.umd.min.js
 15K Apr 22 15:03 react-popper.umd.min.js.map

cc @FezVrasta

…versions, rollup cleanup, and Readme update.

virgofx · 2018-04-24T13:18:54Z

ping @FezVrasta Any chance you can review so we can get this merged with a new build?

virgofx · 2018-04-26T21:09:44Z

@FezVrasta Any chance this can get merged and a new release cut?

FezVrasta · 2018-04-27T16:11:26Z

I don't think I understand the reasoning behind the "full version", it still requires React, React-DOM and Prop-Types. What's the point of having a version tha bundles a single dependency and no all the others?

react-popper never meant to ship its own version of Popper.js, it was a misconfiguration, I'd really like to avoid to have issues open on each Popper.js release asking me to release a new "full version" with the updated Popper.js release...

May you strip out all the changes unrelated to the SSR fix from this PR so we can at least merge that part?

virgofx · 2018-04-27T16:45:16Z

I can do that ... it just makes it easier to drop-in one file instead of two. I only included it that way because it was a regression without understanding that was the original intent. Upstream libraries will now need to be clear to include 2 files (popper.js + react-popper)

FezVrasta · 2018-04-27T17:27:33Z

upstream libraries can define popper.js as dependency and include them by themselves, without having the users to worry about what dependencies are being used under the hood

…sistency/similarity with popper.js dist structure.

virgofx · 2018-04-27T23:39:49Z

Updated. I moved the UMD builds into /dist/umd for consistency with popper.js.

./dist
├── react-popper.js
├── react-popper.js.map
├── react-popper.min.js
├── react-popper.min.js.map
└── umd
    ├── react-popper.js
    ├── react-popper.js.map
    ├── react-popper.min.js
    └── react-popper.min.js.map

virgofx · 2018-04-30T13:47:54Z

ping @FezVrasta

FezVrasta · 2018-04-30T13:49:14Z

Popper.js is a very bad example of dist folder structure 😅

I'd say that, or we keep everything at top level, or we keep each module type in its own folder

virgofx · 2018-04-30T13:51:46Z

You pick --- I was trying to be consistent. Additionally, keeping a module in each folder is best... That's how react does it:

https://unpkg.com/[email protected]/

Otherwise, in the current version .. more people use CJS instead of probably requiring the UMD version just for lack of knowledge of module types.

FezVrasta · 2018-04-30T13:53:02Z

Yeah sorry, I'd like to change Popper.js but it would be a breaking change and people would get pissed off :-/

Let's try to do better here.

…es separated by folder.

virgofx · 2018-04-30T13:58:02Z

./dist
├── cjs
│   ├── react-popper.js
│   ├── react-popper.js.map
│   ├── react-popper.min.js
│   └── react-popper.min.js.map
└── umd
    ├── react-popper.js
    ├── react-popper.js.map
    ├── react-popper.min.js
    └── react-popper.min.js.map

virgofx · 2018-04-30T14:00:26Z

Thanks Fez. Ping me when you cut a release so I can update our upstream libraries.

Also, can I create a new similar PR in master branch (1.0) to re-include the /dist structure similarly as I see it's missing in latest?

FezVrasta · 2018-04-30T14:01:16Z

Yes please, thanks!

virgofx · 2018-04-30T23:57:29Z

Fez can you release 0.10.2 in the meantime when you get a chance? Will update master branch with PR soon. Thanks

FezVrasta · 2018-05-01T09:42:45Z

0.10.2 published

fix: v0.x dist builds to ensure SSR works properly and popper.js is i…

814128a

…ncluded in the dist builds.

FezVrasta reviewed Apr 22, 2018

View reviewed changes

fix: Improve dist builds to include both popper.js and non-popper.js …

cc4b385

…versions, rollup cleanup, and Readme update.

virgofx force-pushed the v0.x branch from eec8d1a to cc4b385 Compare April 22, 2018 15:30

fix: popper.js no longer included. Moved umd builds into /umd for con…

a1f6604

…sistency/similarity with popper.js dist structure.

fix: Ensure /dist folder structure uses best-practice with module typ…

0d1c24e

…es separated by folder.

FezVrasta merged commit fb7d798 into floating-ui:v0.x Apr 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: v0.x dist builds to ensure SSR works properly and popper.js is included in the dist builds #146

fix: v0.x dist builds to ensure SSR works properly and popper.js is included in the dist builds #146

virgofx commented Apr 21, 2018 •

edited

Loading

FezVrasta Apr 22, 2018

virgofx Apr 22, 2018 •

edited

Loading

virgofx commented Apr 22, 2018 •

edited

Loading

virgofx commented Apr 24, 2018

virgofx commented Apr 26, 2018

FezVrasta commented Apr 27, 2018 •

edited

Loading

virgofx commented Apr 27, 2018

FezVrasta commented Apr 27, 2018

virgofx commented Apr 27, 2018 •

edited

Loading

virgofx commented Apr 30, 2018

FezVrasta commented Apr 30, 2018

virgofx commented Apr 30, 2018 •

edited

Loading

FezVrasta commented Apr 30, 2018

virgofx commented Apr 30, 2018

virgofx commented Apr 30, 2018 •

edited

Loading

FezVrasta commented Apr 30, 2018

virgofx commented Apr 30, 2018

FezVrasta commented May 1, 2018

fix: v0.x dist builds to ensure SSR works properly and popper.js is included in the dist builds #146

fix: v0.x dist builds to ensure SSR works properly and popper.js is included in the dist builds #146

Conversation

virgofx commented Apr 21, 2018 • edited Loading

FezVrasta Apr 22, 2018

Choose a reason for hiding this comment

virgofx Apr 22, 2018 • edited Loading

Choose a reason for hiding this comment

virgofx commented Apr 22, 2018 • edited Loading

virgofx commented Apr 24, 2018

virgofx commented Apr 26, 2018

FezVrasta commented Apr 27, 2018 • edited Loading

virgofx commented Apr 27, 2018

FezVrasta commented Apr 27, 2018

virgofx commented Apr 27, 2018 • edited Loading

virgofx commented Apr 30, 2018

FezVrasta commented Apr 30, 2018

virgofx commented Apr 30, 2018 • edited Loading

FezVrasta commented Apr 30, 2018

virgofx commented Apr 30, 2018

virgofx commented Apr 30, 2018 • edited Loading

FezVrasta commented Apr 30, 2018

virgofx commented Apr 30, 2018

FezVrasta commented May 1, 2018

virgofx commented Apr 21, 2018 •

edited

Loading

virgofx Apr 22, 2018 •

edited

Loading

virgofx commented Apr 22, 2018 •

edited

Loading

FezVrasta commented Apr 27, 2018 •

edited

Loading

virgofx commented Apr 27, 2018 •

edited

Loading

virgofx commented Apr 30, 2018 •

edited

Loading

virgofx commented Apr 30, 2018 •

edited

Loading