React-popper: Feature Request: Clarify package contents & allow for better tree shaking

[ericgio]

Hi guys, thanks for the great libraries (both react-popper and popper.js).

I ran into this issue today and couldn't figure out at first why it was happening until I realized that the package contents weren't quite what I expected. I was able to solve the issue, but am no longer able (as far as I can tell) to easily tree shake the components I'm not using.

My request/suggestion is twofold:

1) Change the package folder structure to make it more clear what the contents are. The most common approach I've seen is to provide a dist folder containing the minified and unminified UMD builds, a lib or cjs folder with the individual CJS components, and an es folder with the original ES versions of the components.
2) For both the CJS and ES versions, provide the separate components instead of combining them into a single module so consumers can easily exclude unused code using something like babel-plugin-transform-imports.

I'm happy to submit a PR for this if this is something you're open to. Thanks for considering my request.

[FezVrasta]

Thanks for the issue.

AFAIK CommonJS is not three-shakeable, not being statically analyzable. I don't see much value in providing a per-file basis release of the CJS build. If one cares about bundle size should use the ES build.

[ericgio]

Maybe I'm not using the term correctly :) I simply want to include the component I use (Popper) and not include the other stuff. If the individual CJS components are included in the package, I can do that, but as it stands, I can't.

If one cares about bundle size should use the ES build

Not everyone (or every environment) can consume ES modules, which is what happened in the issue I linked to above. CJS still seems like the most widely compatible way to offer the package and most major libs I've come across provide the code as CJS or both. It also doesn't seem like much overhead to provide both the ES and CJS versions of the individual components, but would provide more options for developers.

[FezVrasta]

Feel free to send a PR please. (possibly on both v0.x and v1.x)

[TheSharpieOne]

and an es folder with the original ES versions of the components.

I personally don't see any value in original/individual esm since treeshaking does work with esm. In the case where there is a single file/bundle which exports all of the components using esm and only importing Popper, you only get Popper; everything is excluded.
I had mentioned something along the lines of having the lib/ directory being cjs and not esm before in https://github.com/souporserious/react-popper/pull/109#issuecomment-376918164 for the reasons you mention above.

[FezVrasta]

The problem is that current bundlers don't work particularly well with single-file ES module bundles. (and Flow neither)

[FezVrasta]

I'm actually taking care of the changes needed in the v1.x/master branch.

Does a package.json like this make sense?

  "main": "lib/cjs/index.js",
  "module": "lib/es/index.js",
  "browser": "dist/react-popper.js",

[FezVrasta]

d3e235981baea5c378aed771e867f2cb421d7d05

[TheSharpieOne]

Yeah, that makes sense, but that would also be a breaking change unless you also kept the current es files in lib/ as well. (since if people depend on react-popper/lib/Popper.js now, it would break).

[FezVrasta]

I'm applying these changes only to the v1.x branch actually.

[TheSharpieOne]

d3e2359 reintroduces what #93 fixed due to the way the lib/cjs/index.js is transpiled/generated and how the exports are defined.

[FezVrasta]

What's the way to go to make Babel transpile those exports properly?

[TheSharpieOne]

Not as elegant, but if you split the importing from the exporting it will work.

import Popper, { placements } from './Popper';
import Manager from './Manager';
import Reference from './Reference';

export { Popper, placements, Manager, Reference };

cjs output:

'use strict';

Object.defineProperty(exports, "__esModule", {
  value: true
});
exports.Reference = exports.Manager = exports.placements = exports.Popper = undefined;

var _Popper = require('./Popper');

var _Popper2 = _interopRequireDefault(_Popper);

var _Manager = require('./Manager');

var _Manager2 = _interopRequireDefault(_Manager);

var _Reference = require('./Reference');

var _Reference2 = _interopRequireDefault(_Reference);

function _interopRequireDefault(obj) { return obj && obj.__esModule ? obj : { default: obj }; }

exports.Popper = _Popper2.default;
exports.placements = _Popper.placements;
exports.Manager = _Manager2.default;
exports.Reference = _Reference2.default;

[ericgio]

Thanks for considering the request. Looks like @FezVrasta's commit addresses the issue, at least for v1.x.