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

[Javascript] Added support for commonjs in nodejs package #4217

Merged
merged 3 commits into from
Apr 13, 2023
Merged

Conversation

Tarjei400
Copy link
Contributor

No package should enforce upon creator a need to migrate to ESM. As per this pull request, I added webpack configuration that produces cjs and mjs files Correspoding to CommonJS and ESM module. After this pull request if will be up to a project which one to use.

Cheers!

Signed-off-by: Adrian Jutrowski <[email protected]>
@ericvergnaud
Copy link
Contributor

Hey, nobody's enforcing anything, and we welcome contributors. Let's see how this works for other devs looking for CommonJS support.

@NilsBaumgartner1994
Copy link

@Tarjei400 thank you :-) will have a look. @ericvergnaud thanks for notification.

@Tarjei400
Copy link
Contributor Author

@enessoylu I just mean that at the moment library requires ESM in a project and this is "enforcing" ESM in a project, I didnt mean anyone per se :P Anyways thanks for taking a look, I think it will be a usefull addition for this package!

@jharris4
Copy link
Contributor

jharris4 commented Apr 3, 2023

I think adding exports to the package.json is a step in the right direction.

Following down this path should help with the TypeScript target as well. For example as of TypeScript 5.0 you can do the following with exports in your package.json to export separate types for node and web:

{
  "exports": {
    ".": {
      "node": {
        "types": "src/index.node.d.ts",
        "default": "dist/bundle.node.js"
      },
      "browser": {
        "types": "src/index.web.d.ts",
        "default": "dist/bundle.web.js"
      }
    }
  }
}

And with TypeScript 5 the package entry point & types can be toggled in the tsconfig.json by changing compilerOptions.moduleResolution between nodenext and bundler.

TypeScript docs

NodeJS docs

@Tarjei400
Copy link
Contributor Author

Tarjei400 commented Apr 3, 2023

@jharris4 I will take a look later this afternoon for all cases you mentioned , however in my case I can only import cjs file in node project.

@Tarjei400
Copy link
Contributor Author

Tarjei400 commented Apr 11, 2023

@jharris4 Sorry it took some time. I checked that export solution and added tests for nodejs esm/commonjs and updated PR

@@ -0,0 +1,31 @@
"use strict";
var __createBinding = (this && this.__createBinding) || (Object.create ? (function(o, m, k, k2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I could use an explanation of the code in this file. Why can't we just write "require('antlr4')" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I just transpiled it with ts, but you are right require will do the job. Changed 👍

@@ -5,68 +5,63 @@ import {fileURLToPath} from "url";
const __filename = fileURLToPath(import.meta.url);
const __dirname = path.dirname(__filename);

const nodeConfig = {

const buildConfig = ( platform, extentions ) => ({
Copy link
Contributor

@ericvergnaud ericvergnaud Apr 12, 2023

Choose a reason for hiding this comment

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

exten*t*ions or extensions ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Type, fixed

@ericvergnaud
Copy link
Contributor

If I understand correctly, this PR would generate esm and cjs for web and node, without touching any runtime code ? Sounds promising! Awaiting for my inline comments to be addressed.

@Tarjei400
Copy link
Contributor Author

Correct @ericvergnaud :D Addressed your comments!

@ericvergnaud
Copy link
Contributor

Good stuff! Thanks so much, I know for sure this will make many developers happy.
@parrt blessed

@Tarjei400
Copy link
Contributor Author

Myself included :D Also thanks for this great tool!

@ericvergnaud
Copy link
Contributor

Fixes #4211

@jedwards1211
Copy link

@Tarjei400 big thanks for this 😊

@NilsBaumgartner1994
Copy link

NilsBaumgartner1994 commented Nov 10, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants