chore: use rollup instead of tsup for a cleaner dist #255

fvsch · 2024-07-19T21:13:58Z

Fixes #216.

This PR removes tsup and adds an equivalent Rollup configuration, in order to improve the structure and verbosity of the dist output.

Issues I’m trying to solve

ESM build

Presence of many chunks at dist/chunk-{hash}.mjs, often one per es-toolkit function, leading to hard-to-browse dist folder and a deeper-than-necessary module graph (dist/index.mjs -> dist/array/index.mjs -> dist/array/unionWith.mjs -> dist/chunk-OG2RLT5U.mjs).
Presence of many bare imports (possibly used as a way to hoist transitive imports and cause the JS engine to import code earlier, but may not be ideal for re-bundling those sources, in addition to being noisy).
Presence of many empty chunks (and bare imports of empty chunks).
CJS build produces dist/browser.mjs (which duplicates umd/browser.global.js and is not exposed in package.json#exports).

CJS build

dist/index.js is a full bundle of the whole toolkit, instead of importing from the rest of the CJS output.
dist/{category}/index.js are bundles for those categories.
dist/{category}/{functionName}.js are also produced (along with their sourcemaps), but those files are not used
CJS build produces dist/{category}/{functionName}.js files and their sourcemaps, but those files are never used (because package.json#exports points to the dist/{category}/index.js files, which are bundles and don't require dist/{category}/{functionName}.js).
CJS build produces dist/browser.js (which duplicates umd/browser.global.js and is not exposed in package.json#exports).

Proposed `dist` layout

dist/
  index.js         # CJS root entrypoint, requires all {category}/index.js
  index.js.map
  index.d.ts       # Declarations for CJS
  index.mjs        # ESM root entrypoint, imports all {category}/index.mjs
  index.mjs.map
  index.d.mts      # Declarations for ESM
  _chunk/
    *-[hash].js    # Small number of chunks produced in the CJS build
    *-[hash].js.map
  {category}/
    index.js       # CJS entrypoint, bundled code (may require from _chunk/*.js)
    index.js.map
    index.d.ts     # Declarations for CJS
    index.mjs      # ESM category entrypoint, imports from {category}/{fn}.mjs
    index.mjs.map
    index.d.mts    # Declarations for ESM
    {fn}.mjs       # ESM function implementation
    {fn}.mjs.map
    {fn}.d.mts     # Declarations for ESM

Things look maybe a bit easier to parse if we filter out by format. Here is the output structure for ESM:

dist/
  index.mjs        # root entrypoint, imports all {category}/index.mjs
  index.mjs.map
  index.d.mts
  {category}/
    index.mjs      # entrypoint, imports from {category}/{fn}.mjs
    index.mjs.map
    index.d.mts
    {fn}.mjs       # function implementation
    {fn}.mjs.map
    {fn}.d.mts

and for CJS:

dist/
  index.js         # root entrypoint, requires all {category}/index.js
  index.js.map
  index.d.ts
  _chunk/
    *-[hash].js    # small number of chunks (only 4 currently)
    *-[hash].js.map
  {category}/
    index.js       # entrypoint, bundled code (may require from _chunk/*.js)
    index.js.map
    index.d.ts

Note: for CJS there is some bundling going on, while for ESM we try to output a transpiled copy of the src dir (without tests) that mirrors its structure. I opted for this because that's what is going on currently with tsup (once you fix some useless files and some excessive indirection created by chunks).

It should be possible to do different things though:

Make {category}/index bundles of all the functions for that category, in both CJS and ESM builds.
Or make both CJS and ESM builds mirror the structure of the src module graph ({category}/index entrypoints import function implementations from {category}/{fn}).

Results of this PR

I managed to get a dist output that mimics the current output, minus the outlined issues.
Size of the generated package.tgz:
- Before: 879 files, 309kB
- After: 528 files, 164kB
This requires roughly 200 LOC of configuration using Rollup, up from ~12 with tsup.
Build times are a bit slower, but not by much: on my M1 Mac, yarn pack with tsup takes 3-4 seconds, and with Rollup it takes 4-5 seconds.

Other concerns

Choice of bundler

The choice of bundler or transpiler is conditioned by the characteristics of this project:

Sources use .ts extensions in import specifiers.
Goal is to produce .js (CJS) and .mjs (ESM) builds.

This is a problem for both tsc and esbuild, since both have limitations (by design or de facto) when it comes to modifying extensions used in import specifiers.

With ESBuild, this can be worked around by using ESBuild to follow the whole module graph, then do code splitting to have it explode the module graph into many files. This looks like what tsup is configuring ESBuild to do. As a result the output has the configured extensions (.js or .mjs) in import specifiers, but a side effect is that we end up with most code in chunks at dist/chunk-[hash].mjs.

It looks like there is no way to get tsup and ESBuild to fulfill the requirements of this project.

When it comes to alternatives:

I’ve used Vite to bundle libraries before. Its library mode is an opinionated configuration for Rollup. This might work, but I suspect it might also fail some requirements.
I knew that Rollup has a reputation of being a very flexible tool for compiling and/or bundling libraries, and I had some limited experience with it (and Vite). It's a lower-level tool than Vite (or tsup), but can be configured to control the output more closely through its options, existing plugins, or even custom plugins if need be.
I haven't investigated swc, Parcel, or others.

Duplication of code

With the 3 builds for this project (ESM, CJS, and the IIFE build in umd/browser.global.js), we end up with a lot of verbatim copies of the same code:

TypeScript declaration files for CJS and ESM have identical content.
- For example, dist/array/difference.d.ts and dist/array/difference.d.mts are identical.
- This is probably not fixable, as this is a requirement of TypeScript itself: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-7.html#packagejson-exports-imports-and-self-referencing
JSDoc comments for a function end up duplicated in many files.
- For example, the source JSDoc comment for the difference function in src/array/difference.ts ends up copied verbatim into:
```
dist/array/index.js          (CJS bundle)
dist/array/difference.mjs    (ESM)
dist/array/difference.d.ts   (CJS)
dist/array/difference.d.mts  (ESM)
```
- It should be possible to use TypeScripts removeComments option to not output comments in the .js/.mjs outputs, or in the .d.ts/.d.mts outputs. My intuition is that having JSDoc comments in the TypeScript declaration files only should be enough, but there might be IDEs or configurations where the IDE doesn't use the TypeScript declaration files but does use JSDoc comments from the imported .js/.mjs sources?
- Edit: I checked the current output from tsup, and it looks like comments are kept in the declaration files but stripped in ESM/CJS modules. I'll update this PR to do the same.
Duplication of the original TypeScript sources:
- Edit: discussion moved to Duplication of TypeScript sources in sourcemaps #285

Testing the output

I checked the generated package.tgz with https://arethetypeswrong.github.io/ and everything is green.

I would still like to verify the changes in this PR further:

It might be possible to use @arethetypeswrong/cli on CI?
See also Testing the dist output? #254: maybe that issue should be addressed first, by landing some integration tests, and this PR should only land after that?

vercel · 2024-07-19T21:14:01Z

@fvsch is attempting to deploy a commit to the Toss Team on Vercel.

A member of the Team first needs to authorize it.

codecov-commenter · 2024-07-19T21:17:01Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.26%. Comparing base (72cc2d8) to head (febc2d9).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #255   +/-   ##
=======================================
  Coverage   99.26%   99.26%           
=======================================
  Files         125      125           
  Lines         819      819           
  Branches      186      186           
=======================================
  Hits          813      813           
  Misses          6        6

raon0211 · 2024-07-20T02:02:51Z

Hello, thanks for your pull request! This would be a great help.

package.json

fvsch

Alright, I think this is pretty solid now.

fvsch · 2024-07-23T09:20:04Z

package.json

@@ -28,6 +28,7 @@
  },
  "files": [
    "dist",
+    "umd",


This fixes publishing the umd/browser.global.js.map sourcemap.

The package.json#browser field will prompt npm/yarn to include umd/browser.global.js in the package, but the related sourcemap is ignored unless explicitly included.

See also: #284

Thanks for your catch!

fvsch · 2024-07-23T09:21:25Z

package.json

-    "tsup": "^8.1.0",
+    "rollup": "^4.19.0",
+    "rollup-plugin-dts": "^6.1.1",
+    "tslib": "^2.6.3",


The TypeScript compiler (used via @rollup/plugin-typescript) required tslib to be installed when compiling to JS (CJS or ESM).

fvsch · 2024-07-23T09:24:49Z

rollup.config.mjs

@@ -0,0 +1,192 @@
+// @ts-check


This is the Rollup configuration.

Rollup uses Node to load this module, and supports .js, .cjs and .mjs, but unlike Vite or tsup it does not support TypeScript.

We can still use TypeScript types (which is quite useful for autocompletion) using // @ts-check and types in JSDoc.

I used ESM, but I’m happy to convert to CJS if that's preferred.

fvsch · 2024-07-23T10:21:08Z

rollup.config.mjs

+import tsPlugin from '@rollup/plugin-typescript';
+import dtsPlugin from 'rollup-plugin-dts';


We're using rollup-plugin-dts in addition to @rollup/plugin-typescript because if we generate both JS outputs and declarations with @rollup/plugin-typescript like this:

const config = { // … plugins: [ tsPlugin({ compilerOptions: { declaration: true, declarationDir: 'dist', } ], { output: 'esm', dir: 'dist', entryFileNames: '[name].mjs', } });

Then we get this output:

dist/ index.mjs index.d.ts

with the declaration file having a .d.ts extension (instead of the desired .d.mts). What's more, those declaration files will contain the original import specifiers from the source files, which I suspect would not work well:

// dist/index.d.ts export * from './array/index.ts'; export * from './error/index.ts'; export * from './function/index.ts'; export * from './math/index.ts'; export * from './object/index.ts'; export * from './predicate/index.ts'; export * from './promise/index.ts'; export * from './string/index.ts';

Finally, these declaration files are emitted by the TypeScript compiler directly to disk. Which means that we cannot use Rollup configuration options, or even a simple custom output plugin, to:

change the file extension;

codify the file contents (in order to change the .ts extensions in import specifiers).

I was stuck on this problem for a long while, then found rollup-plugin-dts which abstracts away this problem by telling tsc to emit declaration files to a temporary folder, then reads back that output and turns it into Rollup inputs. In this process, it handles rewriting the import specifiers with our expected extensions, and we can use Rollup's output.entryFileNames option to set the extension of the declaration files themselves.

So in total we are doing 4 different Rollup builds:

Use @rollup/plugin-typescript to emit ESM code (dist/**/*.mjs).

Use @rollup/plugin-typescript to emit CJS bundles (dist/**/*.js).

Use rollup-plugin-dts to emit declaration files and translate those into both .d.ts and .d.mts with correct import specifiers (.js and .mjs).

Use @rollup/plugin-typescript and @rollup/plugin-terser to emit umd/browser.global.js.

There might be a better solution than using rollup-plugin-dts here, but I haven't found it. The TypeScript compiler itself is responsible for a lot of the difficulty here, since it's a powerful but somewhat inflexible and stubborn piece of software (very Microsoftian ^^).

fvsch · 2024-07-23T10:44:31Z

rollup.config.mjs

+    browserBuildConfig({
+      inputFile: './src/compat/index.ts',
+      outFile: packageJson.publishConfig.browser,
+      name: '_',
+    }),


We can use Rollup to generate the umd/browser.global.js bundle using src/compat/index.ts as an entrypoint, without needing an extra src/browser.ts module.

Here, the { output: { format: 'iife', name: '_' } } configuration will produce a file looking like:

var _ = function (t) { "use strict"; // ... implementation, sets t.functionName = ... }({});

Note that the current src/browser.ts (removed in this PR) adds a _ property to window instead:

window._ = toolkit;

This src/browser.ts creates a couple problems:

It gets picked up by the ESM and CJS builds, and produces unnecessary cruft in the dist output (see https://unpkg.com/browse/es-toolkit@1.13.1/dist/):

File Size

dist/browser.d.mts 13 B

dist/browser.d.ts 13 B

dist/browser.js 31.9 kB

dist/browser.js.map 156 kB

dist/browser.mjs 3.3 kB

dist/browser.mjs.map 360 B

Solutions include:

Keep src/browser.ts and add ignore/exclusion rules in tsconfig.json and/or bundler config.

Or remove src/browser.ts completely and use the bundler to achieve the desired "declare a global var" result.

Using window._ = /* toolkit */ will throw in a Web Worker context (e.g. if loading the browser bundle with importScripts). Solutions include:

using var _ = /* toolkit */

using self._ = /* toolkit */

fvsch · 2024-07-23T12:25:11Z

rollup.config.mjs

+  clearDir('dist');
+  clearDir('umd');
+
+  const entrypoints = Object.values(packageJson.exports).filter(f => /^(\.\/)?src\//.test(f) && f.endsWith('.ts'));


After a bunch of testing: using the paths from package.json#exports (the dev version pointing to the src directory) as inputs to Rollup produces the best results:

The whole module tree gets resolved correctly.

This avoids pulling in test files, and modules not marked as test files but only consumed by tests (e.g. src/compat/_internal/*.ts).

Another approach is to use something like globSync('src/**/*.ts') and then do a lot of filtering, but I found that it's easy to forget something while filtering.

fvsch · 2024-07-23T12:26:52Z

src/browser.ts

-import * as toolkit from './compat/index.ts';
-
-interface Window {
-  _: typeof toolkit;
-}
-
-declare let window: Window;
-window._ = toolkit;


As described above, this is now handled with a Rollup build that looks like:

const config = { input: 'src/compat/index.ts', output: { format: 'iife', name: '_', file: 'umd/browser.global.js', } }

raon0211

This is decent work. Huge thanks to @fvsch !

fvsch mentioned this pull request Jul 19, 2024

Dist contains bare imports of empty chunks in ESM output #216

Closed

raon0211 reviewed Jul 20, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

fvsch force-pushed the tsup-to-rollup branch from 62f7a24 to 1583354 Compare July 22, 2024 16:03

fvsch added 2 commits July 22, 2024 18:03

update tsup to 8.2.0 (no change to dist)

4ae8086

add rollup config to generate dist

7d4bdf7

fvsch force-pushed the tsup-to-rollup branch from 1583354 to 7d4bdf7 Compare July 22, 2024 16:04

fvsch added 6 commits July 22, 2024 18:19

fix browser build config

24f9bc1

browser umd bundle sourcemap should be published

1d992c8

browser bundle is iife, not umd, despite folder name

192dde0

rollup: don't use manualChunks to keep the config a tad simpler

62a5dcb

prepack: use rollup instead of tsup

a767272

remove browser entrypoint from dist

6cebc31

fvsch marked this pull request as ready for review July 22, 2024 19:27

fvsch requested a review from raon0211 July 22, 2024 19:27

fvsch mentioned this pull request Jul 23, 2024

Duplication of TypeScript sources in sourcemaps #285

Closed

fvsch added 5 commits July 23, 2024 10:37

remove comments in .js and .mjs outputs

529ad55

remove src/browser.ts, use equivalent Rollup config

9e7ecfb

Restore original .npmignore

3fea9d3

rollup config: rename umdBuildConfig to browserBuildConfig

2e36b06

rollup: use package.json#exports for esm entrypoints

62ea66f

fvsch commented Jul 23, 2024

View reviewed changes

Merge branch 'main' into tsup-to-rollup

febc2d9

raon0211 previously approved these changes Jul 24, 2024

View reviewed changes

Merge branch 'main' into tsup-to-rollup

75d9a0b

raon0211 dismissed their stale review via 75d9a0b July 24, 2024 15:19

raon0211 added 2 commits July 25, 2024 00:20

chore: package.json

a07a3ef

rollup config

76aae2e

raon0211 added 2 commits July 25, 2024 00:26

sort

459b6e7

lint

0142e9d

raon0211 force-pushed the tsup-to-rollup branch from b24caf2 to 0142e9d Compare July 24, 2024 15:34

raon0211 previously approved these changes Jul 24, 2024

View reviewed changes

prettier

fd84a98

raon0211 dismissed their stale review via fd84a98 July 24, 2024 15:35

raon0211 approved these changes Jul 24, 2024

View reviewed changes

raon0211 merged commit 7b2aa1a into toss:main Jul 24, 2024
6 of 7 checks passed

fvsch deleted the tsup-to-rollup branch July 24, 2024 17:40

This was referenced Jul 24, 2024

chore: move browser build from umd dir to dist #314

Merged

Publishing issues with the browser.global.js build #284

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: use rollup instead of tsup for a cleaner dist #255

chore: use rollup instead of tsup for a cleaner dist #255

fvsch commented Jul 19, 2024 •

edited

Loading

vercel bot commented Jul 19, 2024

codecov-commenter commented Jul 19, 2024 •

edited

Loading

raon0211 commented Jul 20, 2024

fvsch left a comment

fvsch Jul 23, 2024

raon0211 Jul 24, 2024

fvsch Jul 23, 2024

fvsch Jul 23, 2024

fvsch Jul 23, 2024

fvsch Jul 23, 2024

fvsch Jul 23, 2024

fvsch Jul 23, 2024

raon0211 left a comment

		import tsPlugin from '@rollup/plugin-typescript';
		import dtsPlugin from 'rollup-plugin-dts';

File	Size
dist/browser.d.mts	13 B
dist/browser.d.ts	13 B
dist/browser.js	31.9 kB
dist/browser.js.map	156 kB
dist/browser.mjs	3.3 kB
dist/browser.mjs.map	360 B

chore: use rollup instead of tsup for a cleaner dist #255

chore: use rollup instead of tsup for a cleaner dist #255

Conversation

fvsch commented Jul 19, 2024 • edited Loading

Issues I’m trying to solve

ESM build

CJS build

Proposed dist layout

Results of this PR

Other concerns

Choice of bundler

Duplication of code

Testing the output

vercel bot commented Jul 19, 2024

codecov-commenter commented Jul 19, 2024 • edited Loading

Codecov Report

raon0211 commented Jul 20, 2024

fvsch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raon0211 left a comment

Choose a reason for hiding this comment

fvsch commented Jul 19, 2024 •

edited

Loading

Proposed `dist` layout

codecov-commenter commented Jul 19, 2024 •

edited

Loading