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

RFC: ESM project support in NodeProject and TypeScriptProject #3447

Open
1 of 6 tasks
giseburt opened this issue Mar 13, 2024 · 30 comments · May be fixed by #3467
Open
1 of 6 tasks

RFC: ESM project support in NodeProject and TypeScriptProject #3447

giseburt opened this issue Mar 13, 2024 · 30 comments · May be fixed by #3467

Comments

@giseburt
Copy link
Contributor

giseburt commented Mar 13, 2024

Problem: It should be trivial to create a project from NodeProject or TypeScriptProject that supports ESM, because yell and scream all you want, ESM is the future. 🤓

> Note: This is not about the projen project itself, except there are a few bugs in other tools that need to be worked around (see #3388). But I'm not suggesting that the projen code itself be converted to ESM (that would be a different issue, and probably is, I haven't looked).

Proposed high-level API change:

const project = new NodeProject({
  //...
  packageType: NodePackageType.ESM, // ← default: NodePackageType.CJS
});

Requirements:

  • Bare Minimum: Add to NodePackage a way to set type: module in the package.json
    • Currently possible with project.package.addField("type", "module");, but a non-escape-hatch method would be nice, see proposal above
  • Side effects: The tools esbuild, jest, ts-jest, and ts-node all have special configuration for ESM-mode
  • Issues:
    • (TS) ts-node: This is the only issue that involves projen itself, and it only happens in ESM mode when using node 18.19 or newer. Covered in When TypeScript project is configured to ESM, projen commands gets ERR_UNKNOWN_FILE_EXTENSION #3388 and a workaround is described in this comment
      • This is an ugly one, and it's really hurting the ts-node project, since it's effecting projects all over that use it, and are both modernizing their codebase and supporting the latest node versions
      • An alternative is a tool called tsx, but that doesn't do type checking (this is explained in the comment on that issue, along with a workaround)

All the above as tasks, from highest to lowest priority:

  • Add a packageType enum to NodePackageOptions to support the type options of ESM = "module" and CJS = "commonjs"
    • Adjust NodePackage to default packageType to CJS and set type in the package.json when packageType is NOT CJS
    • Change NodePackage to add NODE_OPTIONS=$NODE_OPTIONS --experimental-vm-modules to the test task when packageType === ESM
    • Change TypeScriptProject to adjust ts-jest configuration when packageType === ESM
  • Bundler (esbuild) controls for --format and --banner
  • Fix When TypeScript project is configured to ESM, projen commands gets ERR_UNKNOWN_FILE_EXTENSION #3388

Future work, identified from discussion below, but not required to be part of this effort:

  • Provide support for dual output mode packages, to bundle or compile to both CJS and ESM, as well as configuring package.json accordingly
  • Provide guidance for switching code from CJS → ESM (such as adjusting requires without extensions to imports with extensions)
  • GitHub pipeline run tests on multiple node versions, with and without ESM (might be a requirement for this effort) Update: See Make GitHub pipeline run tests on multiple node versions #3499 and fix: add missing build workflow strategy matrix controls #3500 - just need to add ESM options
  • Go through each TypeScript project type (CDK, CDK8s, etc.) and determine if ESM mode can be enabled, and make adjustments as needed
@giseburt giseburt changed the title RFC: TypeScript support for creating ESM projects RFC: NodeProject and TypeScriptProject support for creating ESM projects Mar 13, 2024
@mrgrain
Copy link
Contributor

mrgrain commented Mar 13, 2024

Nice work! Would love to see this happing. Can you include a proposal for the required API changes (high-level) in the RFC.
E.g. is it going to be new TypeScriptProject({ esm: true})?

Open questions:

  • How are we going to deal with publishing? Will this publish ESM only? Could we (later) introduce different publishing methods like ESM-only, and ESM-and-CJS?
  • Some downstream projects like JsiiProject don't support ESM. How are we going to handle those?

@giseburt
Copy link
Contributor Author

giseburt commented Mar 14, 2024

Thank you.

  • How are we going to deal with publishing?

    • The mechanics available are outlined thoroughly here: Dual CommonJS/ES module packages
      • TL;DR:
      // package.json
      {
        "exports": {
          "import": "./index-module.js",
          "require": "./index-require.cjs"
        },
        "types": "./index.d.ts", // see https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html
        "type": "module"
      }
    • I see two obvious options: Dual bundlers (call esbuild twice, once with --format=esm, once with --format=cjs) or dual compile (this would be good to fold into RFC: Target-project based control of TypescriptConfig from TypeScriptProject #3448)
    • Note: Nothing changes with the ESM switch: We can bundle or compile from CJS to ESM or ESM to CJS, so we could start doing this now
  • Some downstream projects like JsiiProject don't support ESM. How are we going to handle those?

    • It's fully opt-in, so they don't have to use it 😃
    • Do they even know if they support it? Maybe the last time they tried it was difficult and didn't have projen support? 😉

I did an experiment and added the code below to the JsiiProject at 10mi2/tms-projen-projects/.projenrc.ts and it still builds and passes all tests.

I checked and the lib folder doesn't change, but the package.json is stuffed into dist so the "type": "module" is getting propagated. I was able to call projen new --from file://... on it and it worked as it had before.


Here's the code changes in the test:

(You'll need RESET_COMPILER_OPTIONS as well)

// set "esm mode" for the project
project.package.addField("type", "module");

// add `tsx` to avoid issues with `ts-node`
project.addDevDeps("tsx");

const PROJEN_TSCONFIG_FILENAME = "tsconfig.projenrc.json";
if (project.defaultTask) {
  // add a secondary projenrc-specific tsconfig file that doesn't emit JS
  const projenTsconfig = new TypescriptConfig(project, {
    fileName: PROJEN_TSCONFIG_FILENAME,
    include: [
      ".projenrc.ts",
      "projenrc/**/*.ts", // added by projen for tsconfig.dev - gives a place for projenrc included files
    ],
    extends: TypescriptConfigExtends.fromTypescriptConfigs([
      project.tsconfigDev,
    ]),
    compilerOptions: {
      ...RESET_COMPILER_OPTIONS,
      noEmit: true,
      emitDeclarationOnly: false,
    },
  });

  // adjust the projen command to:
  // 1. run tsc to typecheck and syntax check the file
  project.defaultTask.reset(`tsc --project ${projenTsconfig.fileName}`);
  // 2. use the projenrc - specific tsconfig and tsx
  project.defaultTask.exec(
    `tsx --tsconfig ${projenTsconfig.fileName} .projenrc.ts`,
  );
  project.defaultTask.description =
    "Run projen with ts-node/esm (workaround for Node 18.19+ applied)";
}

if (project.jest) {
  project.testTask.env(
    "NODE_OPTIONS",
    "$NODE_OPTIONS --experimental-vm-modules",
  );
}

@giseburt
Copy link
Contributor Author

I edited the description with the new proposal. Spoiler: it's an ENUM!

@mrgrain
Copy link
Contributor

mrgrain commented Mar 15, 2024

  • Some downstream projects like JsiiProject don't support ESM. How are we going to handle those?

    • It's fully opt-in, so they don't have to use it 😃
    • Do they even know if they support it? Maybe the last time they tried it was difficult and didn't have projen support? 😉

I did an experiment and added the code below to the JsiiProject at 10mi2/tms-projen-projects/.projenrc.ts and it still builds and passes all tests.

I checked and the lib folder doesn't change, but the package.json is stuffed into dist so the "type": "module" is getting propagated. I was able to call projen new --from file://... on it and it worked as it had before.

Interesting. My assumption was that "type": "module" requires changes to the tsconfig.json file that jsii to compile. ANd since that file is owned by jsii, we couldn't make any changes. Based on your code snippet, the jsii-flavored TS compiler seems to just ignore that. But does that mean you actually can use ESM code in your JsiiProject?

@giseburt
Copy link
Contributor Author

I have been using ESM-style code in my Jsii project. The taconfig settings say what to compile to, and the package.json+some tsconfig settings tell it what it's coming from.

So this might not have worked even six months ago. The tooling and compatibility just wasn't there. But things have improved rapidly.

That's why I was saying the Jsii project might not even know how close they are to ESM support. And having this type of support where they can just enable it and test might help them and others update.

Any thoughts on the api suggestions I edited into the original post? I can work on a PR (or multiple) over the weekend.

@giseburt giseburt linked a pull request Mar 20, 2024 that will close this issue
@mrgrain
Copy link
Contributor

mrgrain commented Mar 25, 2024

Can you try and gather more feedback from the community on this RFC?

@moltar
Copy link

moltar commented Mar 25, 2024

IMO package type shall not have a direct relation with the output format. Because many tools (esp. esbuild-based ones, such as tsup) can transpile (CJS, ESM) => (CJS, ESM).

@moltar
Copy link

moltar commented Mar 25, 2024

However, the biggest problem I ran into when trying to make an ESM-compatible package, is the fact that all imports now must be imported with a .js extension.

import { foo } from `projen/lib/foo.js`

@moltar
Copy link

moltar commented Mar 25, 2024

And then because projen itself is not ESM, you actually cannot import it into an ESM package.

The only way to make a projen-based ESM package that I found was to have a monorepo with a root projen project being CJS, and then a child monorepo package being an ESM one.

If you try to combine it all into a single package, it'll not work, unless projen itself is converted into ESM. I've spent countless hours shaving this yak, and I have failed to make it work.

Would be happy to help in any way I can on this one, including knowledge sharing, as I have many mental note on this problem.

@moltar
Copy link

moltar commented Mar 25, 2024

One awesome tool that helped to figure out the ESM/CJS exports mess was this one: https://github.com/arethetypeswrong/arethetypeswrong.github.io

It's comprehensive and is made by one of the core TS project members.

It's really not a walk in the park to get a single package to be CJS and ESM compatible without the dual package hazard. But it is possible, especially when using tools like tsup.

@giseburt
Copy link
Contributor Author

IMO package type shall not have a direct relation with the output format. Because many tools (esp. esbuild-based ones, such as tsup) can transpile (CJS, ESM) => (CJS, ESM).

Agreed. I think it would be folly to presume they are the same.

However, the biggest problem I ran into when trying to make an ESM-compatible package, is the fact that all imports now must be imported with a .js extension.

Again, agreed. Total pain.

You can alleviate some of that pain with tooling, such as tsx which uses esbuild, and handles that for you. That gets you local tooling working like .projenrc.ts and CDK src/main.ts, and as long as iterop is working (wouldn’t have even six months ago) then projen or CDK or jsii themselves being cjs doesn’t matter.

And then because projen itself is not ESM, you actually cannot import it into an ESM package.

I don’t believe this to be the case anymore. Not with node 18+ and the right configs (outlined above). I know it wasn’t long ago that that was absolutely the case, and this has been a rough transition period and we all have wasted so much time on the exact things your outlining, but I do believe the tooling is finally there and good enough to use.

I’ll make a branch at https://github.com/10mi2/tms-projen-projects/ with the changes I outline in my earlier comment for you to play with and follow up on this comment shortly.

For now, if you run npx projen new --from @10mi2/tms-projen-projects tms-apollo-graphql-app --sample-type=pothos-prisma you’ll get an ESM projen project. It has all the settings I’ve outlined here already in place. Play with it. It makes a GraphQL server that uses top-level await, has a compete test suite, uses Prisma+SQLite, esbuild, graphql-codegen, etc. It’s not a “hello world” toy project, it’s a dead-serious base project for real projects. And it uses ESM and projen. 😄

@moltar
Copy link

moltar commented Mar 26, 2024

you’ll get an ESM projen project.

I wish!! 😁

👾 postinstall » pre-compile » save-schema | ts-node scripts/saveSchema.ts
TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".ts" for /users/foo/tms-apollo-graphql-app/scripts/saveSchema.ts
    at new NodeError (node:internal/errors:405:5)
    at Object.getFileProtocolModuleFormat [as file:] (node:internal/modules/esm/get_format:99:9)
    at defaultGetFormat (node:internal/modules/esm/get_format:142:36)
    at defaultLoad (node:internal/modules/esm/load:120:20)
    at nextLoad (node:internal/modules/esm/hooks:832:28)
    at load (/users/foo/tms-apollo-graphql-app/node_modules/ts-node/dist/child/child-loader.js:19:122)
    at nextLoad (node:internal/modules/esm/hooks:832:28)
    at Hooks.load (node:internal/modules/esm/hooks:415:26)
    at MessagePort.handleMessage (node:internal/modules/esm/worker:168:24)
    at [nodejs.internal.kHybridDispatch] (node:internal/event_target:807:20) {
  code: 'ERR_UNKNOWN_FILE_EXTENSION'
}
👾 Task "postinstall » pre-compile » save-schema" failed when executing "ts-node scripts/saveSchema.ts" (cwd: /users/foo/tms-apollo-graphql-app)
npm ERR! code 1
npm ERR! path /users/foo/tms-apollo-graphql-app
npm ERR! command failed
npm ERR! command sh -c npx projen postinstall

npm ERR! A complete log of this run can be found in: /users/foo/.npm/_logs/2024-03-26T14_19_05_677Z-debug-0.log
/users/foo/tms-apollo-graphql-app/node_modules/projen/lib/task-runtime.js:155
                    throw new Error(`Task "${this.fullname}" failed when executing "${command}" (cwd: ${(0, path_1.resolve)(cwd ?? this.workdir)})`);
                    ^

Error: Task "install" failed when executing "npm install" (cwd: /users/foo/tms-apollo-graphql-app)
    at new RunTask (/users/foo/tms-apollo-graphql-app/node_modules/projen/lib/task-runtime.js:155:27)
    at TaskRuntime.runTask (/users/foo/tms-apollo-graphql-app/node_modules/projen/lib/task-runtime.js:52:9)
    at NodePackage.installDependencies (/users/foo/tms-apollo-graphql-app/node_modules/projen/lib/javascript/node-package.js:835:17)
    at NodePackage.postSynthesize (/users/foo/tms-apollo-graphql-app/node_modules/projen/lib/javascript/node-package.js:353:18)
    at TmsTSApolloGraphQLProject.synth (/users/foo/tms-apollo-graphql-app/node_modules/projen/lib/project.js:352:22)
    at evalmachine.<anonymous>:19:20
    at Script.runInContext (node:vm:133:12)
    at Object.runInContext (node:vm:279:6)
    at createProject (/users/foo/.npm/_npx/80cfb0dc84733c29/node_modules/projen/lib/projects.js:97:8)
    at Projects.createProject (/users/foo/.npm/_npx/80cfb0dc84733c29/node_modules/projen/lib/projects.js:30:9)

This is exactly the same crap I was fighting for days. I gave up. I rarely do. But this time I said it's just not worth it (yet).

@moltar
Copy link

moltar commented Mar 26, 2024

This was on Node v20.7.0

@moltar
Copy link

moltar commented Mar 26, 2024

Here's a clean repro inside a Docker container:

docker run --rm node:20.7.0 sh -c 'cd ~ && npx --yes projen new --from @10mi2/tms-projen-projects tms-apollo-graphql-app --sample-type=pothos-prisma'

@moltar
Copy link

moltar commented Mar 26, 2024

Just tried it in Node 20.11.1, which is the current latest LTS version and it results in the same error.

@moltar
Copy link

moltar commented Mar 26, 2024

I think if we really want to add ESM support to projen, we need to do the following:

  1. Create a battery of tests (matrix of Node versions, and CJS/ESM/both) for the current state. This would include both projen new and also existing projects with some files. We need to make sure all of these can run projen build successfully.
  2. Then we create an ESM version of these tests, which will, of course, all fail. But then we'll try to make it work.

Otherwise, I think we are running the risk of introducing some changes that might not be backward compatible, and/or not compatible in all Node environments. This is also fine if we decide to do so, but then we need to document those both in docs and perhaps, by setting specific baseline engine versions on the package and/or packages that are minted by projen.

@giseburt
Copy link
Contributor Author

@moltar You hit the core issue in #3388 - specifically cited as a risk above.

That was my code using ts-node, which I've now fixed. I need to do a complete sweep and remove all uses of ts-node and switch to tsc + tsx method. I tested with node v20.11.1, btw.

Please try again. To ensure you get the corrected version:

npx projen new --from '@10mi2/tms-projen-projects@^v0.0.33' tms-apollo-graphql-app --sample-type=pothos-prisma

I was able to then:

npm test

npm run start

Having the GitHub pipeline run tests on multiple node versions would be great, and likely for other reasons than just ESM.

@mrgrain
Copy link
Contributor

mrgrain commented Mar 26, 2024

Having the GitHub pipeline run tests on multiple node versions would be great, and likely for other reasons than just ESM.

That should be a straight forward PR. We will need matrix builds anyway to run tests against Windows.

@moltar
Copy link

moltar commented Mar 26, 2024

Please try again. To ensure you get the corrected version:

Yes, that works!

However, the project does use .js imports:

import { decodeUserID } from "./users.js";

Which is, of course, a matter of pure correctness vs taste.

For me, this was an unacceptable trade-off and one of the biggest annoyances that made me quit in rage this whole yak-shaving debacle.

@giseburt
Copy link
Contributor Author

Then we create an ESM version of these tests, which will, of course, all fail. But then we'll try to make it work.

I don't think we'll find that to be true.

Any case where we hit TypeStrong/ts-node#2094 with for sure fail (node 18.19+, ESM, and tsn-node) and I've determined that it's just too far reaching, so I'm going to adjust my advice to simply move to the tsc + tsx method.

All of the CDK projects use ts-node. I've testing with CDK, CDKtf, and CDK8s so far and they all work fine with adjusting to tsx. (This is not testing them in ESM, just that one change. Science!) I have used CDK in ESM mode with tsx and it work fine.

Some projects types will not yet work with ESM in certain combinations, we expect this. jsii is a great example. There's no point making tests that fail when we try to make a jsii project use ESM. But until we enable them to test by getting past the known common hurdles (instead of us all having matching battle scars) it's not as easy for those projects to move forward.

PS: As promised, I made an ESM-flavored version of my jsii project https://github.com/10mi2/tms-projen-projects/tree/esm-test - and it makes it quite a ways, but not 100%. It builds with jsii, and passes the tests. But I apparently need a new test, since it fails on npx projen new --from file:///<...>/TMSProjenProjects tms-apollo-graphql-app --sample-type=pothos-prisma --package-manager=npm because jsii is only compiling to CJS and not populating package.json properly. I don't think it's an impossible task to get it to work, but not something I'm able to work on ATM.

Error [ERR_REQUIRE_ESM]: require() of ES Module <...>/TMSProjenProjects/lib/index.js from <...>/projen-esm-bugtest/node_modules/projen/lib/projects.js not supported.
index.js is treated as an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which declares all .js files in that package scope as ES modules.
Instead either rename index.js to end in .cjs, change the requiring code to use dynamic import() which is available in all CommonJS modules, or change "type": "module" to "type": "commonjs" in <...>/TMSProjenProjects/package.json to treat all .js files as CommonJS (using .mjs for all ES modules instead).

Again, I consider this more of a reason to do this, so we can all move forward fixing this type of issue.

@giseburt
Copy link
Contributor Author

However, the project does use .js imports:
...
Which is, of course, a matter of pure correctness vs taste.

That's way outside the scope of this conversation, as far as I can tell that ship has fully sailed. Opinion-wise, I agree with you, it's ugly. But it's really irrelevant. All the projects the we depend on (Typescript, node, etc.) have decided that the extension should be there. I believe the general concuss was that not having the extension there was a mistake in the first place. 🤷

I see on point of value: Apparently it helps with tree-shaking, and potentially help with CJS/ESM inter as well. Found in the node docs:

import './legacy-file.cjs';

There are tools that'll add the extensions, it's a one-time thing per-project. At this point I consider that a minor battle I lost (one more small scar), and move on. 😄

@giseburt
Copy link
Contributor Author

biggest annoyances that made me quit in rage this whole yak-shaving debacle

BTW, @moltar, thanks for the discussion and chipping in. It's very helpful.

I think we've all been beaten by this ESM thing at least a couple of times. It was rough going there for a while, and we're not completely out of the storm of this transition, but I think there's light on the horizon. I started this RFC because I have hope, I was finally able to make Typescript projects that reliably support top-level await and other such modern capabilities, using all the standard tools like jest, modern node (18 and 20, currently), esbuild, eslint, prettier, etc. Each of which in the past was a barrier, did quite get it right, or something. We still have some ugly workarounds (NODE_OPTIONS=$NODE_OPTIONS --experimental-vm-modules for example), but not a lot.

I have hope still.

@moltar
Copy link

moltar commented Mar 26, 2024

Then we create an ESM version of these tests, which will, of course, all fail. But then we'll try to make it work.

I don't think we'll find that to be true.

I meant that "by default", as in TDD approach. Make it fail (w/o the change) then make it pass (w/ the change).

Any case where we hit TypeStrong/ts-node#2094 with for sure fail (node 18.19+, ESM, and tsn-node) and I've determined that it's just too far reaching, so I'm going to adjust my advice to simply move to the tsc + tsx method.

All of the CDK projects use ts-node. I've testing with CDK, CDKtf, and CDK8s so far and they all work fine with adjusting to tsx. (This is not testing them in ESM, just that one change. Science!) I have used CDK in ESM mode with tsx and it work fine.

I use tsx everywhere too.

It works great. But there's one nasty caveat.

After Node versions v19.0.0, v18.18.0 need to use --import=module syntax.

https://nodejs.org/api/cli.html#--importmodule

Before that, need to use --require.

@moltar
Copy link

moltar commented Mar 26, 2024

That's way outside the scope of this conversation,

In general, I agree in terms of "opinion" - it is outside the scope of this.

But it might not be 100% out of the scope of the project for some corner cases.

Because, in some instances, SampleFile or SampleDir can produce code that uses imports. Which may then need to be adjusted to match the project type.

@giseburt
Copy link
Contributor Author

After Node versions v19.0.0, v18.18.0 need to use --import=module syntax.

A yes, I avoid using it that way in general. The whole node --import tsx ./file.ts thing is too fragile. I just invert it slightly: tsx ./file.ts, and since I'm allergic to global installs, I use npx tsx ./file.ts.

In package.json scripts that's redundant of course, since it's already running in the "npx" context. That's actually what npx is for, running it like it was a package.json script, so I kinda said that backward.

@giseburt
Copy link
Contributor Author

Because, in some instances, SampleFile or SampleDir can produce code that uses imports. Which may then need to be adjusted to match the project type.

Oh yeah. The fact of the extensions needing to conditionally be added is certainly in scope. 👍

@moltar
Copy link

moltar commented Mar 26, 2024

A yes, I avoid using it that way in general.

There are some cases I found this unavoidable. For example, using with plop:

NODE_OPTIONS="--import tsx --no-warnings" plop ...

Maybe we do not have this kind of limitation with projen, not sure. Just throwing everything I've learned into the mix here. 😁

@mrgrain mrgrain pinned this issue Mar 27, 2024
@mrgrain mrgrain changed the title RFC: NodeProject and TypeScriptProject support for creating ESM projects RFC: ESM project support in NodeProject and TypeScriptProject Mar 27, 2024
@giseburt
Copy link
Contributor Author

Having the GitHub pipeline run tests on multiple node versions would be great

@mrgrain Would you consider that a requirement for this effort?

@giseburt
Copy link
Contributor Author

Update: NVM, see #3499 and corresponding PR #3500

@mrgrain
Copy link
Contributor

mrgrain commented Mar 31, 2024

Having the GitHub pipeline run tests on multiple node versions would be great

@mrgrain Would you consider that a requirement for this effort?

I don't see it as a requirement per se, but it does seem like there are some annoying version issues to work out with ESM

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 a pull request may close this issue.

3 participants