Skip to content

setup(esm): put it as first - #53

Open
AugustinMauroy wants to merge 6 commits into
PrismarineJS:masterfrom
AugustinMauroy:esmify
Open

AugustinMauroy wants to merge 6 commits into
PrismarineJS:masterfrom
AugustinMauroy:esmify

Conversation

@AugustinMauroy

@AugustinMauroy AugustinMauroy commented May 8, 2025 •

Copy link
Copy Markdown
Contributor
  • Tested on 20, 22, 24 ✅
  • sadly if we don't want to break we need cjs wrapper

Note

That's an experiment so it's may not be merged

Comment thread README.md Outdated
Comment thread src/mod.js Outdated
@rom1504

rom1504 commented May 8, 2025

Copy link
Copy Markdown
Member

I think we should move much less files. Otherwise it will be really bad in bigger packages.

@AugustinMauroy

AugustinMauroy commented May 8, 2025 •

Copy link
Copy Markdown
Contributor Author

I think we should move much less files. Otherwise it will be really bad in bigger packages.

Why not merge this pr with the proof that CJS works, then do the following which removes the CJS test

now on npmjs: unpacked size 34.3 kB
now on my branch: unpacked size 47.0 kB

@rom1504

rom1504 commented May 8, 2025

Copy link
Copy Markdown
Member

If we have to move so many files then I would say it doesn't work and we shouldn't migrate

@AugustinMauroy

Copy link
Copy Markdown
Contributor Author

If we have to move so many files then I would say it doesn't work and we shouldn't migrate

AH you mean moving to mod, src ... ?

we can do without bu IMO it's will be better in term of maintainability

@extremeheat

Copy link
Copy Markdown
Member

Most of the changes here are unnecessary. Simply changing module.export with export keyword should be all that's needed

@AugustinMauroy

Copy link
Copy Markdown
Contributor Author

Most of the changes here are unnecessary. Simply changing module.export with export keyword should be all that's needed

The idea of having main mod on ESM is to allowing JSR publish (REF discord discussion)

@extremeheat

Copy link
Copy Markdown
Member

Unless that doesn't support npm packages, I don't understand the comment

@AugustinMauroy

Copy link
Copy Markdown
Contributor Author

Unless that doesn't support npm packages, I don't understand the comment

Could you rephrase your idea because I didn't get it

@rom1504

rom1504 commented May 9, 2025

Copy link
Copy Markdown
Member

Can you change this PR so that it modifies the minimal amount of files and moves none ?

@AugustinMauroy

Copy link
Copy Markdown
Contributor Author

I can ofc

@rom1504

rom1504 commented May 10, 2025

Copy link
Copy Markdown
Member

looks like this specific module has already been supporting esm since 2021 #17

@AugustinMauroy

Copy link
Copy Markdown
Contributor Author

looks like this specific module has already been supporting esm since 2021 #17

Yes, but no, the idea here is to put the esm first. As a reminder, the ultimate goal is to publish packages on JSR in order to move forward with the JS ecosystem.

Comment thread index.js Outdated
AugustinMauroy and others added 2 commits June 5, 2025 09:42
Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com>
@AugustinMauroy

Copy link
Copy Markdown
Contributor Author

bump @rom1504

@extremeheat

Copy link
Copy Markdown
Member

New versions of node.js should be able to import between commonjs and ESM without needing library side changes, as long as one is used consistently over the other. So it may make sense to remove all the ESM exports

@AugustinMauroy

Copy link
Copy Markdown
Contributor Author

New versions of node.js should be able to import between commonjs and ESM without needing library side changes, as long as one is used consistently over the other. So it may make sense to remove all the ESM exports

I din't get your idea.

FYI:

  • CJS imported in CJS => no problem
  • CJS imported in ESM => may cause problems => solution for user const pkg = require("my-pkg"); const foo = pkg
  • ESM imported in CJS => no problem
  • ESM imported in ESM => no problem

So it'w why we should all use ESM for our packages

@rom1504 rom1504 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Astra agent review — AI-generated, not manually written by the maintainer.

Rechecked 744619e and the existing discussion.

The current revision has already reduced the file movement, so the early relocation objections should not be repeated as if they still applied. The remaining maintainer decision is the ESM-first support contract: select the minimum Node runtime, preserve callable require('vec3') plus named/default imports there, and test the published package's runtime and declarations together before replacing the existing dual entry points.

The special module.exports export relies on modern interop behavior; passing on a recent Node alone does not settle older supported consumers. This is a design/API assessment, not a recommendation to reject ESM generally or a fresh package-matrix run.

Skills used: prismarine-architecture-review checked package boundaries and public compatibility; prismarine-geometry-movement-review checked the changed geometry or simulation scope; prismarine-review checked current scope and existing feedback to avoid duplicate threads.

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.

4 participants