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: dual-publish ESM and CJS #8

Closed
wants to merge 1 commit into from

Conversation

markandrus
Copy link

I have a CJS project that uses this library, but I needed to fork it to support CJS in addition to ESM. I did this using tsup. WDYT about merging this into @pg-nano/pg-parser?

Without this change

cd $(mktemp -d)

pnpm init

pnpm add $pg-nano/pg-parser

cat >index.cjs <<EOF
const { parseQuerySync } = require('@pg-nano/pg-parser')

console.log(JSON.stringify(parseQuerySync('SELECT 1'), null, 2))
EOF

cat >index.mjs <<EOF
import { parseQuerySync } from '@pg-nano/pg-parser'

console.log(JSON.stringify(parseQuerySync('SELECT 1'), null, 2))
EOF

node index.cjs
# ❌ Fails with ERR_REQUIRE_ESM

node index.mjs
# ✅ Works

With this change

cd $(mktemp -d)

pnpm init

pnpm link $PATH_TO_BUILT_LOCAL_CHECKOUT_OF_THIS_BRANCH

cat >index.cjs <<EOF
const { parseQuerySync } = require('@pg-nano/pg-parser')

console.log(JSON.stringify(parseQuerySync('SELECT 1'), null, 2))
EOF

cat >index.mjs <<EOF
import { parseQuerySync } from '@pg-nano/pg-parser'

console.log(JSON.stringify(parseQuerySync('SELECT 1'), null, 2))
EOF

node index.cjs
# ✅ Works 

node index.mjs
# ✅ Works

Contents of dist/ after applying this change

$ tree dist
dist
├── ast.d.mts
├── ast.d.ts
├── ast.js
├── ast.mjs
├── binding.d.mts
├── binding.d.ts
├── binding.js
├── binding.mjs
├── index.d.mts
├── index.d.ts
├── index.js
├── index.mjs
├── node.d.mts
├── node.d.ts
├── node.js
├── node.mjs
├── select.d.mts
├── select.d.ts
├── select.js
├── select.mjs
├── typeGuards.d.mts
├── typeGuards.d.ts
├── typeGuards.js
├── typeGuards.mjs
├── walk.d.mts
├── walk.d.ts
├── walk.js
└── walk.mjs

@aleclarson
Copy link

Hey! Sorry for the radio silence. I somehow disabled notifications for “all activity” in this repo. 🤦‍♂️

Now that nodejs/node#51977 is merged (and released I think?), there shouldn't be a need to publish both ESM and CJS. Give that a try and let me know if it works.

@markandrus
Copy link
Author

markandrus commented Oct 26, 2024

No worries! :-)

Indeed, it works with --experimental-require-module, which was unflagged in 23 (and I think might eventually be unflagged in 22).

So I think we could close this PR

image

@markandrus markandrus closed this Oct 26, 2024
@markandrus markandrus deleted the dual-esm-cjs branch October 26, 2024 02:43
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.

2 participants