Added es build, which will get beneficial for es2015 modules aware bu…

[Andarist]

…ndlers like webpack 2+ and rollup

IMHO this should also be cherry-picked to the v1 branch.

This will allow webpack3 to use scope hoisting on this package + ofc tree-shaking will become possible.

[jquense]

Thanks for this! There is a bit of an issue as is. The /lib is actually published as the "root" directory of the package, which is why you can do import TransitionGroup from 'react-transition-group/TransitionGroup' without including the lib bit, essentially if you want es directory published in needs to be in the lib one

[Andarist]

No worries, I've got u covered :) !

Ive just ammended the PR, please take a look. Such structure, although slightly inconvenient (package.json per each additional 'cherry-picked' module), allows correct filed to be required by each bundler.

[jquense]

Is there a reason we can't copy the /es directory into l/ib instead? I really don't want to have to maintain a package.json for each file I export

[Andarist]

we cannot support import TransitionGroup from 'react-transition-group/TransitionGroup' form for es consumers if we copy es to the lib

also i aint sure what package.json is geting published, because it aint the one in the git repository and we need to put module property in it, so it can be interpreted correctly and point to the correct file

[jquense]

we cannot support import TransitionGroup from 'react-transition-group/TransitionGroup' form for es consumers if we copy es to the lib

I'm less concerned about that I think, its still the more uncommon case. Eventually we can flip it. The package.json in the root is the one being published, tho the main field is adjusted to index.js before it's copied there

[taion]

Interesting... I wonder if a separate -es package like Lodash does might make sense here, if you want to keep top-level exports.

[Andarist]

Interesting... I wonder if a separate -es package like Lodash does might make sense here, if you want to keep top-level exports.

I think this only bring more confusion to the table, especially for newcomers. A single package works out of the box for both commonjs and es consumers - it also enables easier migration paths from i.e. webpack1 to the webpack2+. Personally I wouldnt vote for this one.

Personally puting es directory in lib for publication purposes seems a bit off too to me, but I'll adjust the PR however you want. However even if thats more uncommon for people to import things like this, its still what you allow and what you also expose in the README as viable solution - so making it incorrect for es consumers will take people off guard here. For me personally it wont be that much of a matter, as well - Im here to discuss it and whatever we decide Ill know how to correctly use the library in my app. Others? Not so much.

Let me know what you decide that should be done and I'll adjust the PR.

[Andarist]

@taion @jquense any decisions about this?

[jquense]

I'm fine adding this, but we need to add it the lib directory per my comment above :) I don't want to maintain a package.json for each file

[Andarist]

Any comments regarding concerns raised here?

[jquense]

I think their reasonable concerns! I don't think think having folks cherrypick from an es namespace is to much trouble compared to the maintenance burden, and general weirdness of having a package.json and sub directory for each file in the repo.

[Andarist]

Ok, I will rebase and adjust the PR soon.

[Andarist]

@jquense Rebased and it should work. However your release-script is replacing the altPkgRootFolder only in main entry. So in the real package.json main points now to pre-release-script path, while module points to the post-release-script path.

I don't think this release-script was designed with multiple entry points in mind and personally I think you'd be better without it, but that's your choice.

Let me know if everything looks ok :)

[Andarist]

@jquense friendly 🏓 :)

[Andarist]

@silvenon I see you are active lately here, so maybe u'd find some time to comment on this one? 😉

[silvenon]

I don't have experience with ES builds yet, so I want to thread lightly.

Why wouldn't import Transition from 'react-transition-group/Transition' work if we place es inside lib?

[silvenon]

Maybe prepend cross-env to NODE_ENV=production as well, now that we have it installed?

[Andarist]

sure thin, gonna apply this change when we agree on other stuff (would like to make a single, bigger change than multiple small ones)

[silvenon]

Are you sure this shouldn't be lib/es/index.js?

[silvenon]

Oh, now I pieced it together, it should be es/index.js because lib gets published as root.

[Andarist]

Yes, although IMHO publishing lib is obscure and personally I would advocate for removing this and just publish normally pkg root + filter files with "files" entry in package.json

[silvenon]

I'm so glad that you switched to .babelrc.js because I don't trust Babel's env, plus it causes duplication.

[Andarist]

😉 I never have trusted babel's env either, I'm rly glad that I have found this neat trick 😀

[Andarist]

Why wouldn't import Transition from 'react-transition-group/Transition' work if we place es inside lib?

Problem is that thing like react-transition-group/Transition can only resolve to a single file in a normal situation. It first tries to load such file, later index.js from such directory. This is somewhat why "module" got introduced as "standard" field - so module aware bundlers can load other files than those specified in "main" (or by a standard algorithm). It simply acts as override for "main".

The only way to hint bundlers to chose other file for such import is to make Transition a directory and put package.json in it - redirecting "main" and "module" to appropriate files (in cjs or esm format). It looks like this. Such directories can be ofc auto-generated with a simple script. I've proposed this solution (although not with auto-generated files) before in this thread, but other maintainer considered this a maintenance burden. I can only say that we have those files for redux-saga for already over a year or something and I had literally 0 issues about it.

Whole algorithm can be found here (this doesnt consider "module" at all, for now "module" is "bundlers thing").

Don't want to advertise my article, but well... I've written it for a purpose - to explain all this craziness, so I encourage you to read it (click).

[silvenon]

Thanks for explaining it so well! These additional package.json files don't seem so bad, also we can auto-generate them, as you said. I would like to go that route to keep backwards incompatibility. Also, I don't see a problem with having lib and es side by side if nothing would change for users.

I'll take some time to study your blog post and see how we can make all this work with our release process.

[silvenon]

@jquense @taion if we add ES modules without changing how the user consumes the package, so that if they turn ES modules on they just get tree-shaking for free, would that be ok if there's little-to-no maintaining overhead on our part?

[Andarist]

Lib as root approach gives more natural “deep” imports, otherwise users would have to use react-transition-group/lib/Transition. But as I said the same thing can be achieved with additional package.json files (just pointing out the possible reason behind lib as root approach)

[Andarist]

And ofc - this shouldnt be a breaking change for the users in any way :)

[jquense]

I mentioned above that managing a bunch of additional package.json files is not something i really want to do, even tho they can be generated, less is more when it comes to build scripts. If someone wants to do the work there and can produce some reusable, small thing we can maybe stick in it's own package i'd be open to it, since it is a nicer experience, but otherwise i want to side on not having a lot of bespoke builds since i work on a lot of stuff

We do publish lib/ as the root for cleaner imports, since the recommended approach, even with esm builds, is cherry-picking, since tree-shaking is an imperfect optimization

[Andarist]

If someone wants to do the work there and can produce some reusable, small thing we can maybe stick in it's own package i'd be open to it, since it is a nicer experience, but otherwise i want to side on not having a lot of bespoke builds since i work on a lot of stuff

Sure, can work on that 😉

We do publish lib/ as the root for cleaner imports, since the recommended approach, even with esm builds, is cherry-picking, since tree-shaking is an imperfect optimization

With cherry-picking tool we'll be able to ditch lib publishing and provide cleaner imports generated by a tool 👍

[Andarist]

@silvenon what's the status of this? Is there anything I could do to help moving this forward?

[silvenon]

@Andarist thanks for pinging, I need to resolve conflicts if there are any and check for myself if this works correctly, maybe add a test if possible. I guess I'm still insecure about my knowledge in types of JavaScript modules so I don't want to merge something breaking, even though I trust that you know and it's really not a lot of code.

It would help if you could tell me how to test manually whether this works. Is it enough to set modules: false in @babel/preset-env and newest webpack will automatically pick up the ES build?

[TrySound]

mjs way will prevent loading esm from module field in react-lifecycles-compat. Webpack is very opinionated about this.

[Andarist]

@TrySound could u elaborate? Preferably with example

[TrySound]

From what I see package with module field (like material ui in the future) will prefer main field over mjs. Package with mjs (like this one) will prefer main field over module field (in case of react-lifecycles-compat). This makes mjs quite unusable since in most cases you get just cjs.
Webpack sucks.

[TrySound]

I personally don't care a lot because my production bundler is rollup.

[jquense]

Wepback will resolve any mjs file over js given how it's extensions are set for default. What it won't do tho is interop with cjs pretending to be esm tho, it's mjs parsing is strict which means any import that's not mjs is considered to be cjs and can only have default imports. This conflicts with webpacks other resolution logic which prefers the module entry to main. It's all sort of a mess for interop, but webpack is trying to do no harm by implementing mjs logic not to spec or out of line with what node is doing, to help prevent longer term fragmentation

[jquense]

To be slightly cleaerer mjs files in webpack will only consider other mjs files to be esm modules, not esm in .js files

[Andarist]

O damn, this is is a mess then - packages have and will have all sort of dependencies and it's IMHO impractical to require each dependency to use .mjs to be treated as ESM module.

I would personally be in favour of not using .mjs in this situation and just leverage more "standard" (as in - not conflicting with anything) "nested" package.json files, but this was met with hesitance previously.

[jquense]

Yeah sorry @Andarist i realize that I did all this exploration into how this works with other of my own packages and didn't follow back up here.

Let's move to the nested package.json route and get something out? Care to take that on?

[Andarist]

Let's move to the nested package.json route and get something out? Care to take that on?

Yeah, sure! I think @TrySound might have wanted to do that too, could you confirm Bogdan? If you are not going to work on this one I'll probably get to it in few days.

[TrySound]

Sure, I can do it too.