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

esm output is invalid #1941

Closed
samuelstroschein opened this issue May 30, 2023 · 14 comments
Closed

esm output is invalid #1941

samuelstroschein opened this issue May 30, 2023 · 14 comments

Comments

@samuelstroschein
Copy link
Contributor

Problem

The ESM output seems to be invalid. Esbuild throws an error upon bundling chevrotain in an application for platform: neutral:

 [ERROR] Could not resolve "@chevrotain/utils"

    node_modules/chevrotain/lib/src/parse/parser/traits/perf_tracer.js:1:84:
      1 │ ...odash/has";import __import__chevrotain_utils from "@chevrotain/utils";import __import____parser from "../parser";"use ...
        ╵                                                      ~~~~~~~~~~~~~~~~~~~

  The "main" field here was ignored. Main fields must be configured explicitly when using the
  "neutral" platform.

    node_modules/@chevrotain/utils/package.json:14:2:
      14 │   "main": "lib/src/api.js",
         ╵   ~~~~~~

  You can mark the path "@chevrotain/utils" as external to exclude it from the bundle, which will
  remove this error.

Proposal

The bug seems to be an easy fix by adding "exports" to the package.json file. This issue is quite urgent for us, would you accept a PR from me?

@bd82
Copy link
Member

bd82 commented Jun 4, 2023

Thanks for reporting this @samuelstroschein

Adding the exports field metadata seems easy enough.
But it does not seem like the sub-packages are compiled to modules.
so what would exports.import point to?

I wonder what is the mid 2023 best practice for publishing a package which is both ESM + commonjs compatible?

@bd82
Copy link
Member

bd82 commented Jun 4, 2023

I found this: https://www.sensedeep.com/blog/posts/2021/how-to-create-single-source-npm-module.html

But their solution (compiling twice) seems a bit complicated (e.g: generating package.json variants from a template).
and having two variants of the source files will also "mess" with module level state.

@samuelstroschein
Copy link
Contributor Author

samuelstroschein commented Jun 4, 2023

Thoughts while I am on the train:

  1. Why do the sub-packages exist?

The code seems to be tiny/can be bundled with the main package itself to ease compilation/deployment etc.

  1. Solving the CJS and ESM compilation.

It seems like adding exports to all packages (which can contain both ESM and CJS exports) and using two TypeScript configs is sufficient.

@bd82
Copy link
Member

bd82 commented Jun 4, 2023

Why do the sub-packages exist?

The project started as a large single package, and it seems I was trying to slowly split it up into sub-components.

It seems like adding exports to all packages (which can contain both ESM and CJS exports) and using two TypeScript configs is sufficient.

This would probably work as I don't believe any of the sub-packages have module level state that would be duplicated (unlike the root package).

But this will also add complexity as the project already uses a different approach (gen-esm-wrapper) for the root project. Perhaps it is time to re-evaluate being an ESM only module.

But that would require more work (e.g adjusting the old website code to work with ESM)

@samuelstroschein
Copy link
Contributor Author

samuelstroschein commented Jun 4, 2023

The project started as a large single package, and it seems I was trying to slowly split it up into sub-components.

The new package.exports field allows sub-nesting.

"exports": {
  "./utilities": "./src/utilities/index.js"
  "./": "./src/index.js"
}
import * as utilities from "chevrotain/utilities"

What approach would you prefer if I open a PR?

  1. using tsc with two configs for sub-packages [safer]
  2. move the sub-packages into chevrotain [no duplicate build steps, potentially eases maintenance]

I double checked NPM. Not a single sub-package has a dependent that is not chevrotain itself. I take that as an indication that publishing the sub-packages like utils, types, etc. has no benefit to the community. Moving the sub-packages into chevrotrain should ease the maintenance and be a trivial change but with a slight risk that someone somewhere on the web directly imports a sub-package outside of chevrotain.

EDIT: 1 seems more complicated than thought because there appears to be a global build script/I can't simply change the build script on a package level.

@mattbishop
Copy link

I found this post to be useful on how to support both module systems: https://evertpot.com/universal-commonjs-esm-typescript-packages/

@bd82
Copy link
Member

bd82 commented Jun 4, 2023

Thanks for the article @mattbishop

My problem with the double compilation approach is that each compiled version would have its own "state" (file level) and that can cause issues.

There are already some edge cases where Chevrotain's "file level" state can cause issues:

I wonder how relevant is going ESM only nowadays for library authors?

@bd82
Copy link
Member

bd82 commented Jun 4, 2023

samuelstroschein

What approach would you prefer if I open a PR?

None to be honest 😢 ,
I don't want a double duplication step due to:

  1. "file/module" level state issues
  2. Potentially not fully testing one of the compiled artifacts (or running the tests twice).
  3. Avoids multiple hybrid package approaches in the same package.

And I'd rather slowly extract more logic out of the root chevrotain packages into sub-packages instead of adding more inside the monolith.

@bd82
Copy link
Member

bd82 commented Jun 4, 2023

@samuelstroschein

The "perfect" solution would be to switch to ESM only.
However, lets assume it may be:

  1. Too much work currently (updating old legacy jquery (globals) based playground / perf page).
  2. Premature in the current JS eco-system (?)

A possible alternative would be to implement the ESM wrapper approach used in the root chevrotain package for the sub-packages as well.

While this library has not been updated in a while... using the same approach as in the root chevrotain package
Will keep the mono-repo consistent and allow us to defer the evaluation of an ESM only approach to a later date.

@samuelstroschein
Copy link
Contributor Author

samuelstroschein commented Jun 4, 2023

And I'd rather slowly extract more logic out of the root chevrotain packages into sub-packages instead of adding more inside the monolith.

What are you trying to achieve with sub-packages?

A possible alternative would be to implement the ESM wrapper approach used in the root chevrotain package for the sub-packages as well.

Adding exports to the package.json's of th sub-packages could be sufficient to solve this issue.

The sub-packages are invalid JS packages but valid TS packages. TypeScript, and thus esbuild, should be able to resolve TypeScript if a package uses TypeScript itself.

@bd82
Copy link
Member

bd82 commented Jun 5, 2023

What are you trying to achieve with sub-packages?

  1. Better separation of components or logic parts into smaller parts with clear APIs.
  2. Longer term: Potential to distinguish between components needed at end user's runtime vs components needed at design time.
    • e.g: 'dts-gen' is not needed at runtime so it could be potentially not even be included in the main chevrotain package
      to reduce bundled package size.
  3. Separation of Chevrotain development packages (e.g website / types) from Chevrotain runtime/design time.

The sub-packages are invalid JS packages but valid TS packages. TypeScript, and thus esbuild, should be able to resolve TypeScript if a package uses TypeScript itself.

I don't quite understand what is a "valid TypeScript package, because TypeScript is a design-time only tool.
If adding something like the snippet below would easily solve the issue, then sure go ahead...

{
"exports": {
    "types": "./lib/src/api.d.ts",
    "require": "./lib/src/api.js",
  }
}

But do note that there is no esm output for those sub-packages so you can't add the exports.import property.

If you can solve the problem by modifying your esbuild configuration, that may be a valid workaround.
Basically if its an edge case of a specific (non-default) configuration of esbuild, maybe its not important enough to solve
right now, and we can defer this until esm only libraries become more standard in the eco-system and the solution (esm only)
would simplify the maintenance of the library instead of adding complexity (e.g double compilation).

samuelstroschein added a commit to samuelstroschein/chevrotain that referenced this issue Jun 6, 2023
Modern bundler settings require the exports field to be present. See Chevrotain#1941
@samuelstroschein
Copy link
Contributor Author

samuelstroschein commented Jun 6, 2023

@bd82 Thank you for maintaining chevrotain and responding to my inquiries. I opened a PR that addresses the issue and has been tested on my local machine #1943 and bd82/regexp-to-ast#130.

Do you have the time to merge and publish the PRs?


No response is needed for the stuff below. Just trying to help you reduce the maintenance work on Chevrotain.

@samuelstroschein What are you trying to achieve with sub-packages?

@bd82 Better separation of components or logic parts into smaller parts with clear APIs.
Longer term: Potential to distinguish between components needed at end user's runtime vs components needed at design time.
e.g: 'dts-gen' is not needed at runtime so it could be potentially not even be included in the main chevrotain package
to reduce bundled package size.
Separation of Chevrotain development packages (e.g website / types) from Chevrotain runtime/design time.

You seem to be optimizing for something else than maintainability/ease of deployment.

  • the community hasn't used dependent on any @chevrotain/* package, indicating that there is little value in publishing separate packages. The increased maintenance burden on your side seems unjustified
  • you can achieve separation by using nested folders in the main chevrotain package and have one build script, one package json, one deployment pipeline.

bd82 pushed a commit that referenced this issue Jun 10, 2023
Modern bundler settings require the exports field to be present. See #1941
@bd82
Copy link
Member

bd82 commented Jun 11, 2023

Do you have the time to merge and publish the PRs?

Hi @samuelstroschein

I've merged #1943
and for regexp-to-ast I've in the process of pulling it into this repo in this branch.

You seem to be optimizing for something else than maintainability/ease of deployment.

Yeah the transition into a (partial) mono-repo was done at a time I was investing more time into Chevrotian...
And also partially to play with and learn about mono repos and relevant tools (yarn and later pnpm)

Although it does seem to help in pulling in regexp-to-ast from the other repo (reduce number of repos being maintained) in this case.

@samuelstroschein
Copy link
Contributor Author

Closing this because merged.

Thank you @bd82 !

samuelstroschein added a commit to opral/monorepo that referenced this issue Jun 14, 2023
/"neutral" leads to errors if an npm module does not have an "exports" field see Chevrotain/chevrotain#1941 . also typescript itself is not compatible with "neutral"
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

No branches or pull requests

3 participants