Skip to content

Conversation

@matteosacchetto
Copy link
Contributor

@matteosacchetto matteosacchetto commented Sep 14, 2022

With this pull request I am adding the support for both CJS and ESM module export. This will allow for example bundlers like webpack, rollup, vite, ... to threeshake any unnecessary code when writing web apps

To achieve this I ended up using rollup (easy to understand and configure) in combination with esbuild (for its speed)

I also update caniuse-lite to latest version (solved a warning during build) and typescript to latest version (due to an issue of version 4.4.2 (rollup/plugins#983)

This pull request relates to discussion #49
Resolves #79

- rollup
- @rollup/plugin-typescript
- rollup-plugin-dts
- rollup-plugin-esbuild
- esbuild
Following changes:

- update lib and target to es2018
- add esModuleInterop and set to true
- add moduleResolution and set to node
The configuration is designe to build:

- declarations
- cjs module
- esm module
Set the correct entrypoint for cjs and esm modules and update build script to perform a typecheck followed by a rollup build
Update typescript to latest version due to an issue that version 4.4.2 has with rollup.
For reference here is the issue: [Builds hang with typescript 4.4.2](rollup/plugins#983)
Add 'sideEffects: false' to package.json to allow webpack (and possible other bundlers) to threeshake the code
@dolsem
Copy link

dolsem commented Sep 15, 2022

Have you done testing to make sure that the resulting ESM target is tree shakable? Just because it's ESM-compatible doesn't guarantee that it's tree-shakable, so I just want to confirm. Honestly, I don't think bundling everything in a single index.js file is ideal, would be best to leave it as separate files, but as long as it's tree shakable and sourcemaps are generated correctly, it shouldn't matter much.

@matteosacchetto
Copy link
Contributor Author

Have you done testing to make sure that the resulting ESM target is tree shakable? Just because it's ESM-compatible doesn't guarantee that it's tree-shakable, so I just want to confirm. Honestly, I don't think bundling everything in a single index.js file is ideal, would be best to leave it as separate files, but as long as it's tree shakable and sourcemaps are generated correctly, it shouldn't matter much.

I tried it using Vite (so Rollup + esbuild) and it was threeshaking perfectly. If you want to try it out wiht webpack, please feel free to do so. Easiest way to try this out is to clone the branch containing these changes, yarn pack to get the .tar.gz, then create a new webpack based project and install it from the .tar.gz archive, then see if it works with webpack too

Regarding the single file vs multiple file, I opted for single file just to make the lib a bit smaller, but If you think multiple files is preferable then I will update it tomorrow toghether with the tsconfig options

@dolsem
Copy link

dolsem commented Sep 16, 2022

I tried it using Vite (so Rollup + esbuild) and it was threeshaking perfectly. If you want to try it out wiht webpack, please feel free to do so. Easiest way to try this out is to clone the branch containing these changes, yarn pack to get the .tar.gz, then create a new webpack based project and install it from the .tar.gz archive, then see if it works with webpack too

Regarding the single file vs multiple file, I opted for single file just to make the lib a bit smaller, but If you think multiple files is preferable then I will update it tomorrow toghether with the tsconfig options

Okay, thank you.

@matteosacchetto
Copy link
Contributor Author

I can confirm that also with webpack it is three shaking correctly! I've tested it on vue-cli project I have and these are the results

Code added

import * as _ from "radash";
console.log(_.first([1, 2, 3]));

Here are the differences

  • Build size withouth above code
    • chunk-vendors.45b3efd9.js - 269.15 KiB
  • Build size with above code and current version of radash available on npm
    • chunk-vendors.45b3efd9.js - 291.06 KiB
  • Build size with above code and the new version of radash containing the changes in this PR
    • chunk-vendors.45b3efd9.js - 269.26 KiB

Update tsconfig to target 'esnext' instead of 'es2018'
Set option 'preserveModules' to 'true' to keep module structure intact instead of combining everything into a single file.
This way it should be easier for bundlers to three shake unused code
@matteosacchetto
Copy link
Contributor Author

esnext does not work with node 12. Following the mapping reported already previously (https://github.com/microsoft/TypeScript/wiki/Node-Target-Mapping) I will go to es2019 to support node 12 and above

Following the guidelines provided here: https://github.com/microsoft/TypeScript/wiki/Node-Target-Mapping, I set tsconfig target and lib to 'es2019' to support node 12
dolsem
dolsem previously approved these changes Sep 16, 2022
Add exports and specific .cjs and .mjs extensions to make sure node uses the correct module loader
Following documentaion on the official nodejs website: https://nodejs.org/api/packages.html#dual-commonjses-module-packages
@matteosacchetto
Copy link
Contributor Author

No idea why the pipeline failed, it seems to be related to something I did not actually touch (and btw, with same node version test pass locally...). If you can @rayepps, rerun the workflow.

Anyway, I added something with respect to the previous commit, which I will specify here:

  "exports": {
    "import": "./dist/esm/index.mjs",
    "require": "./dist/cjs/index.cjs",
    "types": "./dist/index.d.ts"
  },

I added the exports section and I used the .cjs and .mjs extensions to make sure that node uses the correct loader.
It should be something minor, and three-shaking works, I can confirm that

@gander
Copy link

gander commented Oct 7, 2022

No idea why the pipeline failed, it seems to be related to something I did not actually touch (and btw, with same node version test pass locally...). If you can @rayepps, rerun the workflow.

Anyway, I added something with respect to the previous commit, which I will specify here:

  "exports": {
    "import": "./dist/esm/index.mjs",
    "require": "./dist/cjs/index.cjs",
    "types": "./dist/index.d.ts"
  },

I added the exports section and I used the .cjs and .mjs extensions to make sure that node uses the correct loader. It should be something minor, and three-shaking works, I can confirm that

It is dumb tests, based on time:

FAIL src/tests/async.test.ts (10.249 s)
  ● async module › _.sleep function › returns error when error is thrown

    assert.strictEqual(received, expected)

    Expected value to strictly be equal to:
      1665081140877
    Received:
      1665081140876

    Message:
      expected 1665081140876 to be at least 1665081140877

      216 |       await _.sleep(1000)
      217 |       const after = Date.now()
    > 218 |       assert.isAtLeast(after, before + 1000)
          |              ^
      21[9](https://github.com/rayepps/radash/actions/runs/3199576005/jobs/5225480375#step:5:10) |     })
      220 |   })
      221 |   

      at Object.<anonymous> (src/tests/async.test.ts:218:[14](https://github.com/rayepps/radash/actions/runs/3199576005/jobs/5225480375#step:5:15))```

@sodiray
Copy link
Owner

sodiray commented Oct 14, 2022

@matteosacchetto I love this! I'm so excited to merge it. A bunch of PRs were merged recently so there is a new conflict but it's an easy one in the yarn.lock. I cloned your branch and ran it all locally, did some testing 👍 works perfectly.

@gander I would love to know how to improve that test cus I agree, it's dumb 😃

@matteosacchetto
Copy link
Contributor Author

matteosacchetto commented Oct 14, 2022

@rayepps I have resolved the conflict with the yarn.lock and kept typescript version 4.8.3
Since a couple of weeks back typescript 4.8.4 was released, do you want me to bump the version before merging?

@sodiray
Copy link
Owner

sodiray commented Oct 14, 2022

@matteosacchetto can you run rm yarn.lock && yarn this way you'll just regenerate the yarn.lock (thats what I did locally)

I just published 8.0.0-rc.1 from this branch if you want to install it.

@sodiray
Copy link
Owner

sodiray commented Oct 14, 2022

Screen Shot 2022-10-14 at 9 52 06 AM

This is awesome, I don't understand how/why but this CJS/ESM change brought the package size down by almost half, as well as providing the tree shaking 🤩

@matteosacchetto
Copy link
Contributor Author

@matteosacchetto can you run rm yarn.lock && yarn this way you'll just regenerate the yarn.lock (thats what I did locally)

I just published 8.0.0-rc.1 from this branch if you want to install it.

I run rm yarn.lock && yarn and pushed the changes

@matteosacchetto
Copy link
Contributor Author

Again, issue on the same test mentioned above by @gander

@matteosacchetto
Copy link
Contributor Author

Screen Shot 2022-10-14 at 9 52 06 AM

This is awesome, I don't understand how/why but this CJS/ESM change brought the package size down by almost half, as well as providing the tree shaking star_struck

The main reason of the smaller size is the bump on the tsconfig lib and target, so it is doing less transpilation and taking advantage of more modern JS

@matteosacchetto
Copy link
Contributor Author

One additional cool thing: now it threeshakes also if you do import * as _ from 'radash' 😉

@sodiray
Copy link
Owner

sodiray commented Oct 14, 2022

I made an issue to fix the issue in the tests (#102)

cc: @gander

@sodiray sodiray merged commit cc22b12 into sodiray:master Oct 14, 2022
@gustavopch
Copy link

I'm glad this was merged. When is it going to be released?

@sodiray
Copy link
Owner

sodiray commented Oct 16, 2022

@gustavopch it will go out in 2 days (Monday) in v8.0.0. I'm working on a few other changes for the major release and I also happen to currently be traveling 😅

aleclarson referenced this pull request in radashi-org/radashi Jun 24, 2024
* add: rollup, esbuild and rollup plugins

- rollup
- @rollup/plugin-typescript
- rollup-plugin-dts
- rollup-plugin-esbuild
- esbuild

* update: tsconfig.json

Following changes:

- update lib and target to es2018
- add esModuleInterop and set to true
- add moduleResolution and set to node

* add: rollup config file

The configuration is designe to build:

- declarations
- cjs module
- esm module

* update: package.json exports and scripts

Set the correct entrypoint for cjs and esm modules and update build script to perform a typecheck followed by a rollup build

* update: typescript 4.4.2 -> 4.8.3

Update typescript to latest version due to an issue that version 4.4.2 has with rollup.
For reference here is the issue: [Builds hang with typescript 4.4.2](rollup/plugins#983)

* add: sideEffects to package.json

Add 'sideEffects: false' to package.json to allow webpack (and possible other bundlers) to threeshake the code

* fix: tsconfig target set to 'esnext'

Update tsconfig to target 'esnext' instead of 'es2018'

* fix: set rollup to preserve module structure

Set option 'preserveModules' to 'true' to keep module structure intact instead of combining everything into a single file.
This way it should be easier for bundlers to three shake unused code

* fix: tsconfig target and lib set to 'es2019'

Following the guidelines provided here: https://github.com/microsoft/TypeScript/wiki/Node-Target-Mapping, I set tsconfig target and lib to 'es2019' to support node 12

* fix: set package.json exports and use .cjs and .mjs extensions

Add exports and specific .cjs and .mjs extensions to make sure node uses the correct module loader
Following documentaion on the official nodejs website: https://nodejs.org/api/packages.html#dual-commonjses-module-packages

* build: bump rollup to 2.79.1

* build: regenerate yarn.lock
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

Successfully merging this pull request may close these issues.

Provide ESM option with tree-shakable exports

6 participants