esm output is invalid - Githubissues

samuelstroschein commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Thoughts while I am on the train:

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.

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 commented 1 year ago

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.

https://github.com/Chevrotain/chevrotain/issues/1778

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

samuelstroschein commented 1 year ago

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?

using tsc with two configs for sub-packages [safer]
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 commented 1 year ago

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

bd82 commented 1 year ago

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:

https://github.com/Chevrotain/chevrotain/issues/1056

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

bd82 commented 1 year ago

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:

"file/module" level state issues
Potentially not fully testing one of the compiled artifacts (or running the tests twice).
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 commented 1 year ago

@samuelstroschein

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

Too much work currently (updating old legacy jquery (globals) based playground / perf page).
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.

https://github.com/addaleax/gen-esm-wrapper

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 commented 1 year ago

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 commented 1 year ago

What are you trying to achieve with sub-packages?

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.

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 commented 1 year ago

@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 https://github.com/bd82/regexp-to-ast/pull/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 commented 1 year ago

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 commented 1 year ago

Closing this because merged.

Thank you @bd82 !

Chevrotain / chevrotain

esm output is invalid #1941

Problem

Proposal