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 support to CommonJS #519

Merged
merged 4 commits into from
Jan 29, 2024
Merged

Conversation

guilhermewerner
Copy link
Contributor

This PR adds

Support for CommonJS modules using esbuild.

Closes #517

Explanation

To support projects that use ESModules and CommonJS at the same time, it is possible to use a bundler to transpile the code.

In this PR, esbuild was used, available in the file tools/esbuild.js, when executing this file a lib bundle converted to CommonJS is generated, which can then be used by projects that use require.

So that the npm package knows which file should be used according to the project type, the main directive of package.json was replaced by exports, which determines the two files that will be exported according to the type.

"exports": {
    "import": "./lib/index.js",
    "require": "./dist/index.cjs"
},

The bundler settings can be changed in the script, in this PR the transpiled file is created in the dist/index.cjs folder, however it is up to the lib maintainers to decide where this file should be, just remember that this file was generated by the bundler and It has relatively low readability and should be ignored from the repository.

To facilitate the package publishing process on NPM, scripts were created to run the build automatically when the package is installed (prepare).

The examples were also changed to demonstrate use with require and import.

Detail about the got dependency

The http client library that is used only supports ESModules (sindresorhus/got#1789).
image

To fix this, it is included in the bundle so that esbuild can convert its source code. The downside of this is that the bundle file becomes much larger.

Copy link
Member

@CosminPerRam CosminPerRam left a comment

Choose a reason for hiding this comment

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

LGTM, thanks bunch.

Edit: forgot to mention to add a line in the changelog file.

examples/require.cjs Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
tools/esbuild.js Outdated Show resolved Hide resolved
tools/esbuild.js Outdated Show resolved Hide resolved
@CosminPerRam
Copy link
Member

Thanks bunch, just a note to have them for future references:

Detail about the got dependency

This is unfortunate, but we'll have to go with this for now.

I too think that the abrupt move to ESM wasn't exactly the best choice for v5, but I think it'll be fair to completely remove support for it for v6 (as by then we would already be passing 3 years since stable esm support).

Copy link
Member

@CosminPerRam CosminPerRam left a comment

Choose a reason for hiding this comment

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

Forgot to mention to add a line in the changelog file

@guilhermewerner
Copy link
Contributor Author

Forgot to mention to add a line in the changelog file

I always forget the changelog 🫠

@podrivo
Copy link
Contributor

podrivo commented Jan 29, 2024

@guilhermewerner Sorry, a last minute conflict... :(

@CosminPerRam CosminPerRam merged commit 38ddfd6 into gamedig:master Jan 29, 2024
4 checks passed
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.

feat: Add support for ESModules and CommonJS at the same time
3 participants