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

feat: Add ESM build #23

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: Add ESM build #23

wants to merge 1 commit into from

Conversation

drwpow
Copy link

@drwpow drwpow commented Jan 12, 2025

Fixes #22 by adding an ESM build. This is backwards-compatible! It is only an additive change.

⚠️ This adds a npm run build command that is now necessary to generate the ESM files.

  • Adds types that satisfy TypeScript’s requirements for dual-build packages
  • Updates exports to match both Node.js’ standards as well as TypeScript’s
    • ⚠️ Verify these are backwards-compatible with all consumers! It should be, but verify
  • Improves .npmignore by hiding test files and GitHub cruft from the npm package (which saves considerable network traffic, taking into account how often this package is downloaded!)

Alternatives

  • The codebase could be converted to ESM, instead, with CJS generated as a side-effect. But that alternative wasn’t taken to minimize disruption.
  • A bundler such as Rollup could be introduced to remove the custom one-off script. But that seemed heavier-weight, and would impact CI times far more than the current method

Checklist

{
source: './index.js',
types: './types/index.d.ts',
replacements: [
Copy link
Author

@drwpow drwpow Jan 12, 2025

Choose a reason for hiding this comment

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

Note: RegEx replacements can be brittle in larger setups, but this seemed ideal for this package’s current lightweight setup. It’s easier to modify simple string replacements than burdening this package with some robust AST traversal + mutation + reassembly script… that only replaces a couple lines in the end.

Of course, alternatives can be discussed!

fs.writeFileSync(new URL(source.replace(/\.js$/, '.mjs'), CWD), output)

// write types
fs.copyFileSync(new URL(types, CWD), new URL(types.replace(/\.d\.ts$/, '.d.mts'), CWD))
Copy link
Author

Choose a reason for hiding this comment

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

Note: this seems unnecessary—copying existing .d.ts declarations 1:1. But TypeScript does require all-new declaration types for different file extensions, i.e. it won’t apply index.d.ts to both index.js and index.mjs (not even with exports—I’ve tried). This is unfortunately TypeScript behavior, and it’s not easy to bypass.

Alternately, there could be some clever type forwarding, e.g. export * from './index', but in addition to being harder to autogenerate, something could slip through the cracks compared to copying 1:1

@@ -0,0 +1,284 @@
'use strict'

import { dequal as deepEqual } from 'dequal'
Copy link
Author

Choose a reason for hiding this comment

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

dequal ships ESM, so this package would benefit from upgrading, too

@drwpow drwpow changed the title Add ESM build feat: Add ESM build Jan 12, 2025
@Fdawgs
Copy link
Member

Fdawgs commented Jan 12, 2025

Thanks for the PR @drwpow!
If this is being automatically built via a script then my suggestions would be:

  • Add index.mjs and types/index.d.mts to .gitignore, no point committing something that gets built
  • Add a "prepublish": "npm run build" script or something to that effect
  • Scripts are sorted alphabetically ascending, so build should go at the top etc.

You're going to get pushback for adding an .npmignore due to fastify/skeleton#42.

Obviously wait for @fastify/libraries to chime in before actioning any of this, as my frontend knowledge is... dire.

@Fdawgs Fdawgs requested review from ivan-tymoshenko and a team January 12, 2025 08:52
@mcollina
Copy link
Member

I don't think running any of our libraries in a bundle-less browser environment should be a goal (all bundlers compile cjs... React is cjs).

Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

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

Why is it necessary to duplicate everything?

@drwpow
Copy link
Author

drwpow commented Jan 16, 2025

Y’all can feel free to close this if using ESM (the new language standard) isn’t a goal. That’s fine!

@Fdawgs all good suggestions! I went back-and-forth on committing generated files, just because this package didn’t have a build step (but maybe it should—I noticed some of the declarations have drifted from the runtime)

I don't think running any of our libraries in a bundle-less browser environment should be a goal (all bundlers compile cjs... React is cjs).

May be true now but ESM is the new standard. It’s also useful in edge workers (which may also be bundled behind-the-scenes, but ESM saves work)

Why is it necessary to duplicate everything?

ESM is a different module system entirely. I’d recommend reading about it on Node’s documentation but also if you inspect any universal packages you’ll find it (even dequal)

@mcollina
Copy link
Member

ESM is a different module system entirely. I’d recommend reading about it on Node’s documentation but also if you inspect any universal packages you’ll find it (even dequal)

Ok, I will take a look.

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.

ESM Build
4 participants