-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
make javascript package support both esm & commonjs #4199
Conversation
I've created this new smaller PR derived from #4193 but this time as a branch against I'm not sure if there is some way to get the CI tests to run against this PR, like they did for the original PR against master branch? |
Signed-off-by: Jon Harris <harris.jb@gmail.com>
I fixed the conflicts with dev branch on this PR, and tests are all passing now, except for some failures on cpp-builds for gcc. I don't think that's related to this PR though...? |
Signed-off-by: Jon Harris <harris.jb@gmail.com>
"@types/node": "^18.0.5" | ||
}, | ||
"dependencies": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the dependency is required for 'npm link antlr4' to succeed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, all the tests passed without it. You don't need to actually have a package installed to link it. If that were the case nobody would be able to link packages that weren't published first... :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's not my experience at all
it doesn't need to be installed but it has to be declared
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests are all passing without it, and I use it all the time without first installing, so I guess we have different experiences here :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well it's not like having it breaks anything does it ?
@@ -8,7 +8,6 @@ import { default as context } from './context/index.js'; | |||
import { default as misc } from './misc/index.js'; | |||
import { default as tree } from './tree/index.js'; | |||
import { default as error } from './error/index.js'; | |||
import { default as CharStreams } from './CharStreams.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why on earth would you want to remove this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has a dependency on FileStream
which is node only, so the web version needs to have this removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CharStreams.fromString is the recommended constructor, so another solution needs to be identified
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it won't work outside of node in any case, so that's outside of the scope of this PR I'd say.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this PR won't make it if it removes functionality
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is removing a node dependency from a web index, I don't know what to say. Can you explain how you want to keep node dependencies working outside of node?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just pushed a commit just now to address this point. There is now a CharStreams.web.js
file that is a copy of the node version but with the nodejs fs specific code removed.
No functionality is removed by this PR anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're duplicating code. CharStreams.node.js should reuse the code from CharStreams.web.js, adding the 2 file related entry points.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I can make that change. I duplicated code because it followed the pattern of what was done for the index.node.js
and index.web.js
split.
If we want to keep the code base consistent, doesn't this also mean that the same should be done to the two index files to remove duplication? There is quite a hefty amount of duplication there, and in the type definitions too.
If you can accept this duplication temporarily, I think it might be better to open a separate PR if/after this PR is merged to remove duplication in all places. It'd help limit the scope of this PR, do you agree? :-)
"c8": "^7.12.0", | ||
"compression-webpack-plugin": "^10.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so what happens to compression ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
compression doesn't really make sense here. Webpack is used for shipping web applications primarily, and this project is a library... In any case, this only impacts the umd bundle, which nobody really uses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why build umd if 'nobody really uses it' ? And what makes you think that antlr is not used in web apps ? I do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can remove the umd if you'd like. I basically only left it there because it was there before and I was trying to minimize the changes in this PR.
I created this whole PR since we are using antlr4 on the web, so trust me I know how it's used, and this PR is designed specifically to make antlr4 less painful to consume in a web application by correctly setting up the type
, main
, module
& exports
fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there was no umd before, so not sure I follow...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the bundle you used to create with webpack was also in umd format
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
internally, but exposed as esm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm referring to this umd formatted build output that was pointed to by the package.json browser
field.
@@ -1,12 +1,26 @@ | |||
{ | |||
"name": "antlr4", | |||
"version": "4.12.0", | |||
"type": "module", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you proposing to no longer build a module ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the type
module is now only in the es
directory. Previously this library wasn't actually exporting a module (webpack doesn't output esm modules) but said it was. That was part the motivation of this PR, this kind of stuff breaks builds for other libraries that are consuming antlr4.
Now this package will export both commonjs & esm and support both.
Hi, |
@ericvergnaud thanks for the review comments, happy to address or discuss further if that'll help move this PR forward. :-) |
Well you'd have to explain what problem you are solving, with a concrete example. 'Conforming to standards' is not a compelling reason for changing the build when those standards are legacy and tripling the maintenance required to keep it working in the future is definitely a show stopper... |
The problem I'm solving is that we maintain an editor library for the front-end that uses antlr4 for the grammar. Recently antlr4 changed (in minor version change no less!) from exporting an esm module, to now in the latest version exporting commonjs but saying it's an esm module. We also have lots of unit tests on our project that use node require for antlr, whereas the frontend apps use esm imports. It's been whack a mole lately trying to fix one or the other. Suffice it to say, I got tired of working around this, and figured I was better to contributed back a proper fix and help this project get its JavaScript library using best practices for bundling. TLDR I don't want to care about bundling, and my goal here is for nobody else to have to care about want bundle antlr is exporting either. With this PR, both commonjs & esm are supported. If you look carefully, you'll see that in fact the JS tests are now using esm, and the TS tests are still using commonjs because of the tsconfig telling it to here: antlr4/runtime-testsuite/resources/org/antlr/v4/test/runtime/helpers/tsconfig.json Line 4 in 8dcc652
|
Ok, that's a bit of hyperbole saying this PR "triples the maintenance". I put a lot of my own hard (unpaid) work into this, and I spent quite a few hours cleaning things up to try to make it better & easier to maintain. If I failed in that task, it'd be good if you could share specifics. |
Signed-off-by: Jon Harris <harris.jb@gmail.com>
not sure where you take it that the current build exports a commonjs module when it actually uses esm imports, doesn't tolerate require and doesn't work at all if you remove "type : module"... |
re versioning, the readme tells you that antlr can't follow semver practice. And it didn't change from esm to cjs, rather it got webpacked IIRC. |
so you're telling me that something is not working as expected from an esm module, and your 'fix' is to make antlr available as a cjs one ? |
the reason I say "triple maintenance" is because from the number of attempts you've been through I can anticipate how much pain it's going to be each time anything changes in the tools involved in the build chain. With 1 build chain it's already a pain, you're proposing to have 3, 1 for each module type... |
how about just fixing whatever you think is wrong in esm, and updating your tests to use esm too ? |
I have plenty of esm stuff using the esm build (that you say is commonjs) and it works like a charm. |
I also get the feeling that the problems you encounter relate to your specific situation where the front-end is esm but the back-end is cjs ? or is it the front-end tests ? |
As for the motivation of this PR, I offered to have a chat to discuss the motivation & reasoning behind all these changes. A code walkthrough could really help with that, but here we are in text... Some backstory:
As per your comments about lots of commits getting this working, YOU SURE ARE RIGHT! :-) The actual changes for this PR were mostly done by me in about 20 minutes, and the rest of the time & commits was understanding tests & fixing them since I couldn't see how to run them locally. This PR also makes an improvement in that the JavaScript platform test now uses the code that is published to npm instead of just the source, which is a better practice. I'd prefer to see that this PR LESSENS the maintenance burden because it's bringing the JavaScript library's build system more in line with best practices as well as fixing some issues. If you'd like to see the kind of thing this PR is trying to solve, here's an example of the kind of workaround we'd like to stop using to be able to use this repository: https://github.com/neo4j/cypher-editor/blob/main/packages/antlr4-browser/src/index.js TLDR on above example, without this we can't use antlr4 for the web. So please don't assume I'm inventing problems here that don't exist. Believe me I'd very much like to be able to not think about it again, but if it'd be helpful, I'd be happy to help maintain the JS build on this package. |
Also, FYI unkpg can be really useful for examining package build output. For example here is the current main entrypoint for antrl4: https://unpkg.com/browse/antlr4@4.12.0/dist/antlr4.node.js You can see from the above (which is the build output from the JavaScript package) that the code is not pure esm. it's some weird webpack generated thing that imports the require function from a node module... |
The current build follows official webpack practice for esm modules, taken from here: AFAIK the build works fine for both node and web, thanks to webpack's multi-module feature. I understand that you have a specific need to generate a commonjs library, not just an esm one. I'd welcome a cjs build but it can't be at the cost of breaking the simplicity of the current build and dropping features or compression. Since cjs is dead anyway, why not stick to your currently working solution (antlr4-browser) until you no longer need to ship cjs ? |
I understand webpack quite well, I've contributed complex PRs to it, and I maintain a popular open source plugin for webpack to help with deploying web apps. :-) That said, webpack has What tools like webpack (or vite) are VERY good at, is when you are building a web application they scan libraries in node_modules and optimize their bundling if possible (using techniques like tree shaking for esm mode). For this optimized bundling to work properly, library packages should have Currently, the antlr4 package only has this:
And with this PR it now will have this:
If this package wants to be consumable by both node and web, there is no other documented way to make this work that I'm aware of. As for commonjs support. Believe me, I would LOVE to move into a world where it's not needed anymore. The trouble is, we are maintaining packages for OTHERS to use. For example with our cypher editor, we have users who are still using commonjs so we can't drop support yet unfortunately. In general, if you look at any major package published to npm for use in the web right now, you'll see they all provide both cjs & esm entry points to try and play nice with people who are consuming them in their builds. I'm interested to hear more about |
Signed-off-by: Jon Harris <harris.jb@gmail.com>
Those imports are bringing in the entire default import, so it's not just the Lexer, it's all the functionality a consuming web library such as our cypher editor needs. So the savings are considerable! But in any case, it seems like we're reaching the end of the road here. I feel it's a shame that my hard work and commitment to making things better wasn't better received, but I guess part of that is on me for failing to make the case for why this PR is more than just a spurious improvement. In any case, I guess it's not a total waste since I now see an upgrade path for us from Thanks for all your great work on this library, I really do appreciate that it builds properly for node & web now in the latest versions thanks to your improvements. If you want to discuss this PR further I'd be happy to, but I also understand if you'd just rather stick with stuff you're comfortable with. |
Indeed, thanks for your efforts. Please close. |
ok closing, it'll be around as a reference for anyone else who wants to look into this stuff :-) I'd consider making another PR to just fix the esm exports (without adding cjs), if there's any interest in having a fix for that merged. If this library is open to that possibility let me know, otherwise I won't spend the effort. |
stuck at the same problem currently:
|
Unfortunately this only works for react apps but not for plain node projects. Following this simple tutorial to create a npm package: https://itnext.io/step-by-step-building-and-publishing-an-npm-typescript-package-44fe7164964c And having this simple "index.ts" file:
results in the same error. |
Why don't you use import { InputStream } from 'antlr4' ? |
Because it still show the same error. Would be realy nice if antlr4 would support this as mentioned in this PR.
I will check your other comment
|
Yeah, this example is trying to use commonjs node module resolution to import antlr4 which afaik just won't work since it currently has This PR fixes the above error (and makes the esm build sizes smaller too), but I seem to have done a poor job explaining why the fix was important enough to be merged. I wish we were in a pure esm world where we could get rid of commonjs, but too many of our consumers are still using commonjs for us to make that choice for them. It's a tough sell telling them they have to go modernize all their JavaScript to esm from cjs just to use our code, but without this PR we don't seem to have any other options. It's so strange to hate commonjs and want to get rid of it, and be here trying to argue for it's inclusion in a PR, but here we are! haha |
I think transitioning to modern projects is good. But I think what would help here would be for people a clean small example package. For me I want to develop a npm package which has typescript and can be imported and used in other projects like react-native. Therefore the building process was a bit hard and to have a possibility to test my package. This makes transitions easier if you have a MWP (minimal working project). |
Feel free to propose a sample MWP that can be downloaded from the docs. |
Trying to make it work |
Can use antlr4 in directus because of esm package: directus/directus#17997 |
@jharris4 trying out your npm package for antlr4 since i hope it will solve my problem. Unfortunately it has no documentation yet ;-) But i believe that should be easy doable. |
@jharris4 Your package works like a charm <3 Your points are exaclty correct.
That is so 100% true. @ericvergnaud I hope you guys see that this is a real need :-) I love antlr4 since it is very powerful. |
For all out there facing a similar problem this package could help from @jharris4 |
@NilsBaumgartner1994 thanks for confirming the fix in the forked package, but please note that package was purely for testing, and not recommended for real world usage. People should stick with the official |
Hi there, Sure but as you said, it shows that the fix works.Beste Grüße,Nils BaumgartnerAm 30.03.2023 um 14:19 schrieb Jon Harris ***@***.***>:
@NilsBaumgartner1994 thanks for confirming the fix in the forked package, but please note that package was purely for testing, and not recommended for real world usage.
People should stick with the official antlr4 package with the caveat that there's no support for commonjs module resolution.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Looking forward to official antlr4 support.Beste Grüße,Nils BaumgartnerAm 30.03.2023 um 14:19 schrieb Jon Harris ***@***.***>:
@NilsBaumgartner1994 thanks for confirming the fix in the forked package, but please note that package was purely for testing, and not recommended for real world usage.
People should stick with the official antlr4 package with the caveat that there's no support for commonjs module resolution.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
We have a similar problem: Our frontend application (written in TS) is using @reduxjs/toolkit and other dependencies which are not available as ESM. |
Which antlr4 version did you used prior?Beste Grüße,Nils BaumgartnerAm 31.03.2023 um 14:09 schrieb Antonis Balasas ***@***.***>:
We have a similar problem:
Our frontend application (written in TS) is using @reduxjs/toolkit and other dependencies which are not available as ESM.
Upgrading antlr to 4.12 causes tests that use the package to fail. To fix them I followed Jest's guide on testing ESM modules which caused tests that were using non-ESM modules to fail from their side.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
@NilsBaumgartner1994 using 4.8.0 using js/babel, no TS |
Please move this to the CommonJS support discussion |
@antoniom @NilsBaumgartner1994 please check #4217 |
@jharris4 You're my lifesaver, thanks for your NPM package! I was spending half a day trying to get the ANTLR4 runtime running without the ESM error, then I saw the comment from @NilsBaumgartner1994, just in a half minute of installing the package then I got rid all issues! :D |
You're welcome :-) looking forward for official support as this is a thing more people need. |
No description provided.