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

Convert package to standard JS modules #318

Merged
merged 4 commits into from
Apr 3, 2022
Merged

Conversation

justinfagnani
Copy link
Contributor

Fixes #246

Note: This is definitely a major breaking change!

There are ways to make this less of a breaking change and publish both CommonJS and JS modules, but this PR contains the simplest way to publish modules if you can ask your users to upgrade. It is of course important to acknowledge that importing standard JS modules into CommonJS is difficult and requires using dynamic import() or a bundler.

I split this into two main commits. The first simply exports the Chess function as-is. The second reorganizes some things to be a little more idiomatic for modules and puts constants in the top-level scope of the module. This means that KING, PAWN, etc can be imported like:

import {Chess, KING} from 'chess.js';

which solves a question previously in the code about how to best export public constants.

I also considered converting Chess to a real class, but am saving that for later if you're interested.

Exports the public constants, solving the question in the public constants section of how best to handle them.

This should hopefully leave only functions that access instance state in the Chess class.
@jhlywa
Copy link
Owner

jhlywa commented Mar 26, 2022

Thanks @justinfagnani! Would it be helpful to publish this as 1.0.0-alpha? I'd like to make a few API changes prior to 1.0.0 and I'm unsure if changing the API between 1.0.0-alpha and 1.0.0 is bad form.

@justinfagnani
Copy link
Contributor Author

I think you could do this as 0.13, since in semver pre-1.0 minor version bumps are for breaking changes.

I do think it's really worth considering how current users will feel about publishing as module-only. I'm a modules-maximalist and this is simpler, so I'd be for it personally, but people who use this in CommonJS will have a hard time importing it. I could set up a build system to create both JS modules and CommonJS and use package exports to let both importer styles get the right files. That would only work in versions of Node that support package exports, but that's probably less disruptive than requiring users to change their CommonJS to standard modules.

If you wanted me to do that I might consider writing porting to TypeScript at the same time and using the TypeScript compiler to output CommonJS... but I don't know how you feel about all of that :)

@jhlywa
Copy link
Owner

jhlywa commented Mar 30, 2022

I'll need to spend some time investigating the impacts of this change wrt the community. chess.js has been major version 0.x.x for 13 years (which is an eternity in JS years - the coding style is a dead giveaway), so I think the users are expecting a stable API even though it's major version 0. Adhering to the principle of least surprise, does npm/yarn prevent minor version updates for caret deps if they're pre-1.0.0? As long as the upgrade to 0.13.0 is explicit, I'm definitely in favor of merging this and cutting a new release.

I've been working on a private typescript rewrite on and off (honestly, mostly off) for the past year. The current typescript branch was a first attempt, but it's more of a false start, so please ignore. I should really push my latest, publish a roadmap, and seek some feedback/input/PR's before going too much further.

@jhlywa
Copy link
Owner

jhlywa commented Apr 1, 2022

I haven't verified this, but pre-1.0.0 caret updates appear to be explicit (https://docs.npmjs.com/cli/v6/commands/npm-update#caret-dependencies-below-100). I'll try to confirm it with verdaccio this weekend.

Additionally, a basic roadmap for 1.0.0 has been published at #319

@jhlywa jhlywa merged commit 215ea3a into jhlywa:master Apr 3, 2022
@jhlywa
Copy link
Owner

jhlywa commented Apr 4, 2022

The version upgrade pre-1.0.0 is explicit (when crossing minor versions), so this upgrade should be safe. v0.13.0 has been published to npm. Thanks for the PR!

@GabrielDelepine
Copy link
Contributor

type: "module" has been removed from package.json in this commit 37ac2af#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519

Please re-consider @jhlywa

(Ideally, you will want to adopt the exports field)

@jhlywa
Copy link
Owner

jhlywa commented Feb 1, 2023

Ah, good catch @GabrielDelepine . Thank you

I'm unfamiliar with the exports field. I'll need to read up a bit first before adding

@GabrielDelepine
Copy link
Contributor

GabrielDelepine commented Feb 1, 2023

Thanks for the quick fix @jhlywa but there is currently a major inconsistency in v1.0.0-beta.2

I opened a PR to fix it #374

Side note: contrary to what I said in my last message, you don't especially need to use the exports field because the current stable version of chess.js is only published as module and the code base works fine for both NodeJS and the browsers. I don't know if @justinfagnani would agree with that but I think so

@jhlywa
Copy link
Owner

jhlywa commented Apr 18, 2023

Sorry all, I had to revert the library back to CommonJS - too many users were encountering node issues. I'm open to trying this again in the future, but I'll definitely need help ensure support for both node and the browsers.

@GabrielDelepine
Copy link
Contributor

I totally understand your decision. ESM is giving birth in pain due to some incompatibilities with CommonJS.

The exports field of package.json is the best way to go to have a dual CommonJS + ESM package but I totally understand if it's not on top of your todo list right now

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.

Publish standard modules to npm
3 participants