Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide public ES modules #21

Closed
bisubus opened this issue Oct 8, 2018 · 13 comments
Closed

Provide public ES modules #21

bisubus opened this issue Oct 8, 2018 · 13 comments

Comments

@bisubus
Copy link

bisubus commented Oct 8, 2018

/es package entry point is conventional in modern libraries.

The suggestion is to provide rambdax/es imports in ES5 and ES module format , similarly to how it was done with ramda/es. Notice that Ramda modules are transpiled to ES5 to skip transpilation on bundling, and there is index reimporting file.

Unlike rambda/lib, rambdax doesn't contain separately built CJS modules.

Currently it's possible to import modules as rambdax/src/debounce. The problem is that src is ES2017 and isn't published to NPM, it's also not a good habit to rely on internal package structure.

@bisubus bisubus changed the title Provide ES module imports Provide public ES modules Oct 8, 2018
@selfrefactor
Copy link
Owner

I understand the issue. If adding src folder to NPM helps, then I can do that. The other suggestion is a bit blurry to me, so you either have to provide a bit more details or open PR, which I will accept.

@bisubus
Copy link
Author

bisubus commented Oct 12, 2018

Thanks for the attention. Adding src may help and would be a good thing. That's only a part of the problem. Since src is ES2017, it forces a user to transpile package code while it could be just imported. This results in more cumbersome builds in my experience.

I was in the middle of PR when noticed that I overlooked /dist/rambda.m.js which already does that (ES modules with ES5) and could be used for tree-shaking. Since rambdax/dist/rambda.m import looks odd, I'd suggest to create /es.js alias for it to comply with import * as R from rambdax/es` convention, but the rest may work as is.

The problem I noticed with both my build of /es and /dist/rambda.m.js is that tree-shaking is awful when rambdax/dist/rambda.m bundle is imported. Seems to be a problem of Microbundlem which is currently used in rambdax, affects rambda as well.

Here's an example:

import R as * from 'rambdax/dist/rambdax.m'
R.pickBy();

npx microbundle -i index.js -o dist --sourcemap false results in 3.8Kb (1.5Kb gzipped) bundle, which is way too large for pickBy. It should be x20 smaller.

I found out that there are transpiled classes (namely Switchem) in the bundle that aren't related to a module in use (pickBy) but aren't dropped by the minifier because they are transpiled to IIFEs and considered side-effects. Seems to be this problem. But Microbundle uses Buble which doesn't seem that advanced to cover that. I'm not sure if the problem can be fixed without ditching Microbundle and switching to Rollup+Babel 7. Any thoughts on that?

@selfrefactor
Copy link
Owner

selfrefactor commented Oct 12, 2018

So the issue is actually blocked by tree-shaking.

For me babel is example why many developers hate Javascript

The number of times they change the very base of the library is way too many. So for me was no surprise that when I try to upgrade my working rollup configuration, it resulted many babel related errors. And that is the reason why I switched to microbundle in rambda and rambdax as it was painful debugging.

For now, I will expose src with the next version and meanwhile I will try to find some solution for tree-shaking.

@bisubus
Copy link
Author

bisubus commented Oct 13, 2018

In the end, yes, proper tree-shaking is the reason why devs may prefer ES module import instead of other bundles.

I'm not very fond of Babel for that reason, too. Typescript generates less spec-compliant but slimmer and cleaner code and can be used as plain JS transpiler, though it may have its own concerns with tree-shaking. I did a PR to Buble, hope this can provide a simple way to solve this.

BTW, the package itself has Babel dev dependencies. They are currently unused, aren't they?

@selfrefactor
Copy link
Owner

I believe babel dependencies are leftovers from the previous setup. I will try upgrading microbundle as that may solve the issue. Otherwise I will try to make rollup works.

@selfrefactor
Copy link
Owner

I will have to close this issue.
I played with rollup in order to fix tree-shaking, but Rambdax remains unshakable. For the time being, this is not something that can be easily solved. In the future there are some ugly options, that may be applied, but for now this is not the case.

@bisubus
Copy link
Author

bisubus commented Oct 15, 2018

I would expect that this will be naturally solved with new Buble release. Cannot test for now if this is true, but from now it seems that class annotations were the only obstacle. Of course, this should be addressed in both Rambda and Rambdax in order to work.
I'm working with Rollup setups aimed for tree-shaking, so I'm positive the problem is solvable.

@selfrefactor
Copy link
Owner

Great to hear that. I do hope that you manage to reach a solution. Feel free to reopen this when that happens.

@selfrefactor
Copy link
Owner

new Rambda version will solve this, when new Rambdax will use Rambda as development dependency..

@bisubus
Copy link
Author

bisubus commented Oct 21, 2018

Thanks for your efforts. Yes, it is built as intended with new setup.

There's a bug, it should be ./dist/rambdax.esm.js instead of ./dist/rambdax.es.js, https://github.com/selfrefactor/rambdax/blob/master/files/rollup.config.js#L12 . It's listed as rambdax.esm.js in package.json.

I have a couple of suggestions on current setup. Since rambda and rambdax are different packages it would be better to not include it into cjs build (I guess that's the consequence of having it as dev dependency in src), otherwise it could be discarded in favour of UMD module. And for umd, it's good to have unminified .umd.js and minified .umd.min.js. There are no source maps but they would be usable for all dist modules.

@selfrefactor selfrefactor reopened this Oct 21, 2018
@selfrefactor
Copy link
Owner

The bug is a nice catch, I was wandering why there is some issue with tree-shaking. It is fixed now. Also sourcemaps are included in the latest release.

I am not sure what you mean by Since rambda and rambdax are different packages it would be better to not include it into cjs build

Now Rambda stays as dev dependency only in order to be used in support scripts. It has no direct connection to the build process. The latest major change in Rambdax was to copy all of Rambda's src folder into src/rambda and change all import statements.

@bisubus
Copy link
Author

bisubus commented Oct 22, 2018

I mean that if one of project dependency uses Rambda and another uses Rambdax, this will result in two Rambda copies. Not a problem for me because I use Rambdax and tree-shake it, but since Rambda is relatively popular, this seems possible.

The latest major change in Rambdax was to copy all of Rambda's src folder into src/rambda and change all import statements

I wonder if this was necessary. Considering that Rambda switched to Rollup+Babel too, I suppose that treeshaking will now work with external rambda dep and export * from 'rambda' that was previously used.

Usually there are reasons to keep separate UMD and CJS builds if CJS can partially rely on external deps and contain require('rambda'), while UMD contains bundled rambda. At least that's how UMD and CJS builds differ in other libs like React.

@selfrefactor
Copy link
Owner

selfrefactor commented Oct 22, 2018

I wonder if this was necessary. Considering that Rambda switched to Rollup+Babel too, I suppose that treeshaking will now work with external rambda dep and export * from 'rambda' that was previously used.

My test with tree-shaking showed me that it doesn't work as great this way, that is why I went in this direction.

I agree for this case when there are two copies of rambda, but it is a downside I am willing to take.

You rightly pointed that it is challenge to maintain two similar libraries and at one point I had enough of it. I feel that this is better option as Rambda is relatively stable and doesn't change much.

My main task was to make development in Rambdax easier for me and that was the main reasoning behind removing Ramda as dependency.

I will close the issue, but you can comment further if you find that necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants