tannerntannern / ts-mixer

A small TypeScript library that provides tolerable Mixin functionality.
MIT License
379 stars 27 forks source link

Provide ES module instead of commonJS #36

Closed hakimio closed 3 years ago

hakimio commented 3 years ago

It would be nice if there was es module build of ts-mixer. Starting with Angular v10, commonJS dependencies produce "CommonJS or AMD dependencies can cause optimization bailouts" warning when building the application and we could potentially reduce bundle size when using es module. Can you provide "ts-mixer" packaged as ES module?

tannerntannern commented 3 years ago

Hi @hakimio, I would certainly consider this but I'm very busy in the coming weeks so I'm not sure when I'd have a chance to look at this. I'd certainly accept a PR! It would likely just involve modifying the yarn build command to output es6 with commonjs, probably in separate folders, so might need to change the main and module fields of the package.json as well. But again, I'd have to dig into it more and I'm not sure when I'll have time.

hakimio commented 3 years ago

Hi @tannerntannern I could create a pull request next week based on this tutorial if you are too busy to do it. Basically we would have both ES and CJS modules plus package.json would look like this:

  "main": "./lib/cjs/index.js",
  "module": "./lib/esm/index.js",

Does that sound good? Or do you prefer to only have ES module?

tannerntannern commented 3 years ago

I'm having a hard time deciding how I want to handle this. Having separate cjs and esm folders would be best for backwards-compatibility, but now we're nearly doubling the size of the package for those who aren't using tree-shaking bundlers (or aren't using bundlers at all). Doing only ES modules would keep the size down for all cases, but will break things for people using vanilla Node.js.

I don't want to just shoot down this suggestion, but I am wondering whether what we're trying to achieve is worth the complications. This package is already reasonably small compared to many common packages. Additionally, bundlephobia lists ts-mixer as "tree-shakable" despite using CommonJS. As I understand it, most bundlers have gotten smarter in recent years and can tree shake CJS modules that don't make use of dynamic imports (which ts-mixer does not, since it's compiled from TS ES modules).

Again, not shooting it down, but I'm skeptical that this is something we want to move forward with. I'm still open to hearing more arguments.

hakimio commented 3 years ago

Doing only ES modules would keep the size down for all cases, but will break things for people using vanilla Node.js.

NodeJS supports ES modules since version 12 released in April 2019, 2 years ago. I don't think this is a good reason to keep commonJS. We could just do a new major release dropping cjs support and people will just have to update their nodejs version.

tannerntannern commented 3 years ago

NodeJS supports ES modules since version 12 released in April 2019, 2 years ago

I believe v12 still required explicitly enabling support via an experimental flag, but yes Node support for ESM has been around for some time now... A major release with breaking changes is always an option, but I'm hesitant to do it just for the sake of being more modern. There needs to be a good reason to impose mandatory updates on users, and I'm not sure I'm seeing one at the moment.

CommonJS is tree-shakable with most modern bundlers, or plugins are available to enable it (webpack-common-shake for example). And even if it wasn't, this package really is quite small, despite having grown larger in recent versions. I'll admit that that last point seems dismissive, but statistically speaking ts-mixer is not likely going to be the most significant source of bundle bloat compared to other modules.

My point is that if you _want- to tree-shake ts-mixer, you can, and if you're stuck with old bundling tools, ts-mixer shouldn't drastically impact your bundle size (on average).

No matter what I do, I am going to create a headache for someone. And if I have to choose who that will be, I'd rather it be the person who has the luxury of using recent node versions/modern bundlers over the person who is stuck using old node versions and/or no bundlers, because the former likely has more options at their disposal.

hakimio commented 3 years ago

Ok, then let's look at another point you are making.

we're nearly doubling the size of the package for those who aren't using tree-shaking bundlers (or aren't using bundlers at all)

If you are using it on the server side, package size doesn't matter and if you are using it on client side and care about app size, you are using/or should use some kind of a bundler. firebase, one of the most popular packages developed by google, recently moved to dual (cjs + es module) distribution. Nobody is complaining about that.

tannerntannern commented 3 years ago

If you are using it on the server side, package size doesn't matter and if you are you using it on client side and care about app size, you are using/or should use some kind of a bundler.

That is definitely a good point. I don't quite agree with "[server side] package size doesn't matter", but it definitely matters less than client side. At any rate, you might have convinced me that going with split cjs/esm folders shouldn't be so bad. I still don't think dropping support for commonjs is a good option, but that shouldn't be relevant for our discussion moving forward.

If you want to take a shot at this, I'd certainly review the PR. Another thing maybe worth considering is splitting out types in addition to cjs and esm, so the type declarations don't need to be duplicated, cutting down on the size increase. What do you think?

hakimio commented 3 years ago

Ok, I'll try to make a PR tomorrow.

tannerntannern commented 3 years ago

37 and #38 have been included in v5.4.1. Thanks for the contribution!