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

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

Merged
merged 20 commits into from
Jul 24, 2024
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
},
"files": [
"dist",
"umd",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your catch!

"*.d.ts"
],
"publishConfig": {
Expand Down Expand Up @@ -136,6 +137,8 @@
"@babel/preset-typescript": "^7.24.1",
"@changesets/changelog-github": "^0.5.0",
"@changesets/cli": "^2.27.1",
"@rollup/plugin-terser": "^0.4.4",
"@rollup/plugin-typescript": "^11.1.6",
"@types/babel__core": "^7",
"@types/babel__preset-env": "^7",
"@types/broken-link-checker": "^0",
Expand All @@ -148,14 +151,16 @@
"eslint-config-prettier": "^8.5.0",
"eslint-plugin-jsdoc": "^48.5.0",
"prettier": "^3.2.5",
"tsup": "^8.1.0",
"rollup": "^4.19.0",
"rollup-plugin-dts": "^6.1.1",
"tslib": "^2.6.3",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"typescript": "^5.4.5",
"vitest": "^1.5.2"
},
"sideEffects": false,
"scripts": {
"prepack": "yarn build",
"build": "tsup && ./.scripts/postbuild.sh",
"build": "rollup -c rollup.config.mjs && ./.scripts/postbuild.sh",
"test": "vitest run --coverage --typecheck",
"bench": "vitest bench",
"lint": "eslint ./src --ext .ts",
Expand Down
172 changes: 172 additions & 0 deletions rollup.config.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,172 @@
// @ts-check
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


import fs from 'node:fs';
import { createRequire } from 'node:module';
import path from 'node:path';
import terserPlugin from '@rollup/plugin-terser';
import tsPlugin from '@rollup/plugin-typescript';
import dtsPlugin from 'rollup-plugin-dts';
Comment on lines +8 to +9
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Use @rollup/plugin-typescript to emit ESM code (dist/**/*.mjs).
  2. Use @rollup/plugin-typescript to emit CJS bundles (dist/**/*.js).
  3. 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).
  4. 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 ^^).


/**
* @type {{
* exports: Record<string, string>;
* publishConfig: { browser: string };
* }}
*/
const packageJson = createRequire(import.meta.url)('./package.json');

const testPatterns = ['**/*.bench.ts', '**/*.spec.ts', '**/*.test.ts'];

export default () => {
clearDir('dist');
clearDir('umd');

const entrypoints = Object.values(packageJson.exports).filter(f => /^(\.\/)?src\//.test(f) && f.endsWith('.ts'));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


return [
libBuildOptions({
format: 'esm',
extension: 'mjs',
entrypoints,
outDir: 'dist',
}),
libBuildOptions({
format: 'cjs',
extension: 'js',
entrypoints,
outDir: 'dist',
}),
declarationOptions({
entrypoints,
outDir: 'dist',
}),
browserBuildConfig({
inputFile: './src/compat/index.ts',
outFile: packageJson.publishConfig.browser,
name: '_',
}),
Comment on lines +48 to +52
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.
  1. 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 */

];
};

/**
* @type {(options: {
* entrypoints: string[];
* format: 'esm' | 'cjs';
* extension: 'js' | 'cjs' | 'mjs';
* outDir: string;
* }) => import('rollup').RollupOptions}
*/
function libBuildOptions({ entrypoints, extension, format, outDir }) {
const isESM = format === 'esm';

return {
input: mapInputs(entrypoints),
plugins: [
tsPlugin({
exclude: [...testPatterns],
compilerOptions: {
sourceMap: true,
inlineSources: true,
declaration: false,
removeComments: true,
},
}),
],
output: {
format,
dir: outDir,
...fileNames(extension),
// Using preserveModules disables bundling and the creation of chunks,
// leading to a result that is a mirror of the input module graph.
preserveModules: isESM,
sourcemap: true,
generatedCode: 'es2015',
// Hoisting transitive imports adds bare imports in modules,
// which can make imports by JS runtimes slightly faster,
// but makes the generated code harder to follow.
hoistTransitiveImports: false,
},
};
}

/**
* @type {(options: {inputFile: string; outFile: string; name: string}) => import('rollup').RollupOptions}
*/
function browserBuildConfig({ inputFile, outFile, name }) {
return {
input: inputFile,
plugins: [
tsPlugin({
exclude: [...testPatterns],
compilerOptions: {
sourceMap: true,
inlineSources: true,
removeComments: true,
declaration: false,
},
}),
],
output: {
plugins: [terserPlugin()],
format: 'iife',
name,
file: outFile,
sourcemap: true,
generatedCode: 'es2015',
},
};
}

/**
* @type {(options: {entrypoints: string[]; outDir: string}) => import('rollup').RollupOptions}
*/
function declarationOptions({ entrypoints, outDir }) {
return {
plugins: [dtsPlugin()],
input: mapInputs(entrypoints),
output: [
{
format: 'esm',
dir: outDir,
generatedCode: 'es2015',
...fileNames('d.mts'),
preserveModules: true,
preserveModulesRoot: 'src',
},
{
format: 'cjs',
dir: outDir,
generatedCode: 'es2015',
...fileNames('d.ts'),
preserveModules: true,
preserveModulesRoot: 'src',
},
],
};
}

/** @type {(srcFiles: string[]) => Record<string, string>} */
function mapInputs(srcFiles) {
return Object.fromEntries(
srcFiles.map(file => [
file.replace(/^(\.\/)?src\//, '').replace(/\.[cm]?(js|ts)$/, ''),
path.join(import.meta.dirname, file),
])
);
}

function fileNames(extension = 'js') {
return {
entryFileNames: `[name].${extension}`,
chunkFileNames: `_chunk/[name]-[hash:6].${extension}`,
};
}

/** @type {(dir: string) => void} */
function clearDir(dir) {
const dirPath = path.join(import.meta.dirname, dir);
if (dir && fs.existsSync(dirPath)) {
fs.rmSync(dirPath, { recursive: true, force: true });
console.log(`cleared: ${dir}`);
}
}
8 changes: 0 additions & 8 deletions src/browser.ts

This file was deleted.

20 changes: 0 additions & 20 deletions tsup.config.ts

This file was deleted.

1 change: 0 additions & 1 deletion vitest.config.mts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ export default defineConfig({
coverage: {
provider: 'istanbul',
include: ['src/**/*'],
exclude: ['src/browser.ts'],
},
},
});
Loading