Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork3
feat: Add ESM build#23
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| { | ||
| source: './index.js', | ||
| types: './types/index.d.ts', | ||
| replacements: [ |
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Note: thisseems 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 applyindex.d.ts to bothindex.jsandindex.mjs (not even withexports—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' | |||
There was a problem hiding this comment.
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
Fdawgs commentedJan 12, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Thanks for the PR@drwpow!
You're going to get pushback for adding an Obviously wait for @fastify/libraries to chime in before actioning any of this, as my frontend knowledge is... dire. |
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). |
There was a problem hiding this 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?
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)
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)
ESM is a different module system entirely. I’d recommend reading about it onNode’s documentation but also if you inspect any universal packages you’ll find it (evendequal) |
Ok, I will take a look. |
Hi, thanks for your PR and interest! I don't think we should do this unless there is a need as Node will probably never remove CJS and it's supported by pretty much everyone I understand this won't be a satisfactory answer for you since ESM isthe standard, I get that, but this is just how things are Luckily, withnodejs/node#51977, maybe we can simply move to ESM one day with most packages |
Uh oh!
There was an error while loading.Please reload this page.
Fixes#22 by adding an ESM build.This is backwards-compatible! It is only an additive change.
exportsto match bothNode.js’ standards as well asTypeScript’s.npmignoreby 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
Checklist
npm run testandnpm run benchmarkand theCode of conduct