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

feat: migrate to ESM #29

Merged
merged 9 commits into from
Aug 12, 2023
Merged

feat: migrate to ESM #29

merged 9 commits into from
Aug 12, 2023

Conversation

CMCDragonkai
Copy link
Member

@CMCDragonkai CMCDragonkai commented Aug 11, 2023

Description

ESM native should be ready, and PKE sort of needs this to be the case. And we had a long-standing issue in MatrixAI/TypeScript-Demo-Lib#32 to bring ESM and its features like top level await into PK.

However there are some weird things to take care of here. Firstly some resources:

Here's some things that indicate the current state of things.

  1. The new "exports" key in package.json is able to specify specific export paths.
  2. The new "imports" key in package.json is able to specify internal import paths.
  3. The compiler tsc does not understand "imports" paths. If you want to use "imports", it has to be redefined as path mapping using "paths" in tsconfig.json. This may be solved by Support import maps and bare import specifiers microsoft/TypeScript#43326
  4. The ts-node does not understand "paths" mapping, which requires bringing in tsconfig-paths, however tsconfig-paths doesn't understand ESM so it cannot be used anymore. So either ts-node is never used with aliases, or we have to swap to using something else.
  5. The benches and tests are supposed to use aliases to avoid too much nested routes. And this requires going up one directory too.
  6. If you use "imports" the paths have to be specified like #*.js". And they have to point to ./dist/*.js. This actually supports nested paths, but ts-node does not support nested paths, only explicit paths.
  7. If you use "imports" and it points to ./dist. Then that means using node runtime, testing and running benches all run against the compiled ./dist. This actually makes sense, because we would want to test and bench against the compiled code, not the src code which has to be compiled on the fly. If we change to doing this, we end up having to change how we bench and test things. We will need to ensure compilation has occurred already beforehand. This should be doable though.
  8. If we don't use "imports", we can continue using path aliases @ that is still defined in tsconfig.json. If we do use "imports", the path aliases have to be redefined the same as the "imports". However, they will point to ./src not ./dist so that the IDE understands it with respect to the ./src. But when we test, they have to switch to resolving to ./dist, and not ./src... if we don't set this up, that could mean that during testing, the typecheck is applied to the src, but then the actual runtime execution is applied to the dist, which can be very confusing.

It seems... that the ideal case would be something like this:

  • Use "imports" with # prefix as the new internal import specifier. This allows the usage of #index.js and #Logger.js in our .ts code.
  • Stop using ts-node, maybe switch to using tsx https://github.com/esbuild-kit/tsx as it claims that it can support all of this.
  • We have to continue to use paths aliases because tsc doesn't understand "imports" yet automatically. These aliases have to point to src in order to ensure that during development type checking is done against the src.
  • During testing with jest which is using swc, it should be able to ignore the path aliases in tsconfig.json. Because #index.js is now a legitimate import specifier. And in particular it will be loading dist/index.js. This means that npm run test must run npm run build first. That makes test a sort of build command prior. This can slow down testing a bit, but it would technically be more accurate.
  • Should "imports" then be used in src? It can be because as long as the downstream node is updated enough, it can understand how to load internal aliases too. But this is risky... cause "imports" is pretty complicated, and not many tools understand it. I'm veering off to still avoiding any aliased paths inside src.
  • If we are going to have to compile before testing or benching, it would a good idea to directly use swc and skip typechecking. Regular builds can still use tsc when we do npm run build, but an incremental swc build prior to testing or benching should be pretty quick.

Issues Fixed

Tasks

  • 1. Get ESM integrated into development, testing and benchmarking
  • 2. Test the package build and using the "exports"
  • 3. Decide whether to use "imports"

Final checklist

  • Domain specific tests
  • Full tests
  • Updated inline-comment documentation
  • Lint fixed
  • Squash and rebased
  • Sanity check the final build

@ghost
Copy link

ghost commented Aug 11, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@CMCDragonkai
Copy link
Member Author

Instead of changing our entire workflow to compile to dist then test and bench (although this would be nice), I've just removed ts-node in favour of tsx, it seems to work fine for now. That being said, tsx uses esbuild which is different from swc and doesn't support some typescript feature which could cause problems later. I think mostly in decorators.

jest.config.mjs Outdated Show resolved Hide resolved
@CMCDragonkai
Copy link
Member Author

Do note that this means:

  • tsx uses esbuild to run benchmarks.
  • jest uses the swc transformer for transpilation while using ts-jest for module mapping
  • tsc is being used to actually build the final output

This does introduce possible variance between benching and testing.

Right now tsx is required to be able to deal with ESM. It forces the usage of esbuild. If we wanted to make it consistent, jest could be changed to using esbuild, and instead of tsc, one could use esbuild.

But for now this this works.

Just need to test out how the package does the exports.

@CMCDragonkai
Copy link
Member Author

It's fine for the benches to rely on a build first, and we probably want to continue using tsc for builds.

But the problem is that for testing, it's not clean or quick unless we keep incremental.

One way would be to not delete the dist during testing, and just use incremental all the time.

That way we can reintroduce "imports" keys which does in fact import against dist instead and the same thing can happen with benches.

@CMCDragonkai
Copy link
Member Author

So by using # instead, and making sure that tsc -p ./tsconfig.build.json is done before benching or testing, it simplified things and we no longer use a module name mapper at all.

It would be important that downstream code, including bundlers understand what #index.js is though. However I'm still erring on the side of caution in terms of not using it inside the src.

Now I've removed ts-jest too. Now benches and tests work against the compiled dist thus ensuring a consistent testing and benching against the compiled code.

Furthermore, we also add in skipLibCheck to improve compile times. I think we enabled it before, but I think it's ok not to check library types during compilation anymore.

@CMCDragonkai CMCDragonkai self-assigned this Aug 11, 2023
@CMCDragonkai
Copy link
Member Author

Testing it in Polykey-Enterprise. This appears to solve the immediate typing issue.

import Logger, { StreamHandler } from '@matrixai/logger';

Next I want to see how the "exports" is handled, and also whether having 1 package ESM is still importable by other CJS packages.

@CMCDragonkai
Copy link
Member Author

Ok further testing... in PKE is interesting.

This:

import Logger, { ConsoleErrHandler } from '@matrixai/logger';
import * as l from '@matrixai/logger';
import { formatting } from '@matrixai/logger';
import StreamHandler from '@matrixai/logger/handlers/StreamHandler.js';
import { ConsoleOutHandler } from '@matrixai/logger';

console.log(
  Logger,
  ConsoleErrHandler,
  l,
  formatting,
  StreamHandler,
  ConsoleOutHandler,
);

Works now. You can see in particular import StreamHandler from '@matrixai/logger/handlers/StreamHandler.js';.

Also this is all we need:

  "exports": {
    ".": "./dist/index.js",
    "./*.js": "./dist/*.js"
  },

The ./*.js is also recursive which simplifies things.

The other thing, is that ts-node does not work. Even with esm enabled. And this is true whether swc is enabled or not.

We had to switch to tsx for it properly understand the exports key.

One issue is that right now I'm getting:

image

In vscode. Need to find out why vscode doesn't understand exports paths. It's possible because I'm using npm link maybe.

@CMCDragonkai
Copy link
Member Author

I have a feeling that if JSON were distributed, you would want to allow that too. I wonder if that would work.

@CMCDragonkai
Copy link
Member Author

Yep it works.

  "exports": {
    ".": "./dist/index.js",
    "./*": "./dist/*"
  },

Allows to import JSON too from subpackages.

import testJSON from '@matrixai/logger/test.json';

But I think to be accurate you should be doing:

import testJSON from '@matrixai/logger/test.json' assert { type: 'json' };

@CMCDragonkai
Copy link
Member Author

We can default to:

    "./*": "./dist/*"

And then do more specific paths which we can set to null if we want to hide some exports:

    "./*": "./dist/*"
    "./internal/*": null

@CMCDragonkai
Copy link
Member Author

CMCDragonkai commented Aug 12, 2023

With these resources...

Ok this is what I ended up with.

  "type": "module",
  "exports": {
    ".": {
      "types": "./dist/index.d.ts",
      "import": "./dist/index.js"
    },
    "./*.js": {
      "types": "./dist/*.d.ts",
      "import": "./dist/*.js"
    },
    "./*": "./dist/*"
  },
  "imports": {
    "#*": "./dist/*"
  },

The types is a now conditional export subkey. That takes over from the top-level types key. I believe by default this is already the behaviour, but it's better to be explicit here.

Now the main and types top-level properties have been removed. These would only be useful if we were to export regular CJS modules or older node runtimes that don't understand the new exports key. We're going to try to avoid doing too much backwards compatibility.

So this works quite well as it supports both .js files, the root .js file, and also non-js files with the last import enabling .json imports.

Additionally if we wanted to export the package.json, we can add that in too, this would ensure that you can do import packageJSON from 'package/package.json';. Which would be routed to the root of the project instead of the dist. Basically don't write another package.json inside src then.

Furthermore, originally discovered that # is not sufficient as an import key. But self-referencing the package is usable with the full package name. So this is possible import Logger from '@matrixai/logger'; inside the src code files.

Finally all downstream projects need to replace ts-node with tsx instead.

So we now have:

  "type": "module",
  "exports": {
    "./package.json": "./package.json",
    ".": {
      "types": "./dist/index.d.ts",
      "import": "./dist/index.js"
    },
    "./*.js": {
      "types": "./dist/*.d.ts",
      "import": "./dist/*.js"
    },
    "./*": "./dist/*"
  },
  "imports": {
    "#*": "./dist/*"
  },

@CMCDragonkai
Copy link
Member Author

It's also necessary for tsconfig.json to do these 2:

    "moduleResolution": "NodeNext",
    "module": "ESNext",

@CMCDragonkai
Copy link
Member Author

It's also necessary for types to come first.

@CMCDragonkai
Copy link
Member Author

Ok attempting to us the current js-logger inside js-db fails without any other changes except npm link ../js-logger.

In particular the error is simply that it cannot bind the module @matrixai/logger. This is probably because the lack of main and types.

So I tried adding:

  "types": "./dist/index.d.ts",
  "main": "./dist/index.js",

Back in, but it did not work.

Adding in:

    "moduleResolution": "NodeNext",
    "module": "ESNext",

Still didn't work.

Now I'm thinking to update my core dependencies to match what I have in js-logger.

Ok so I think it's not backwards compatible. The new js-logger produces ESM code while the current jest tests produce CJS code.

Inside js-db, it will compile to using CJS code, in which case it tries to require CJS modules, but because the referenced code is an ESM, it won't work.

Ok I think because I didn't produce CJS code, it's not possible for downstream CJS projects to depend on it. There are some hacks, but we are trying to avoid too much backwards compatibility. So it does seem changing to ESM right now is going to be infectious, all projects are going to need to change over to it.

I'm going to try this with js-db and see how that goes with jest too.

@CMCDragonkai
Copy link
Member Author

Remember that js-db has other CJS dependencies, so a question becomes will it work if js-db is ESM and js-logger is ESM, but the other dependencies are not.

@CMCDragonkai
Copy link
Member Author

Ok after doing all of that, using tsx is fine. But jest still can't find the ESM module.

I can confirm that node also finds the right module.

But it's not possible to load @matrixai/logger. Something about Jest still needs something.

@CMCDragonkai
Copy link
Member Author

It looks like this problem https://stackoverflow.com/questions/74069138/node-js-experimental-vm-modules-command-line-option-vs-type-module-in-pac.

Jest is still not fully automatic ESM. It relies on a special flag option.

NODE_OPTIONS=--experimental-vm-modules

But even then there's more configuration to figure out. The problem is the usage of swc as well.

@CMCDragonkai
Copy link
Member Author

According to https://jestjs.io/docs/ecmascript-modules and https://github.com/swc-project/jest#q-jest-uses-commonjs-by-default-but-i-want-to-use-esm

I have to enable this option:

    "test": "tsc -p ./tsconfig.build.json && NODE_OPTIONS=--experimental-vm-modules jest",

Which allows jest to actually load ESM.

Additionally once we use ESM, the jest global is now loaded automatically. So the tests/setup.ts now has to have:

import { jest } from '@jest/globals';

globalThis.jest = jest;

That ensures jest is available by setupFilesAfterEnv.

Running this on js-logger works but we get an additional warning:

(node:3596770) ExperimentalWarning: VM Modules is an experimental feature and might change at any time

@CMCDragonkai
Copy link
Member Author

Now when using swc it should automatically be compiling to ESM because package.json has "type": "module".

@CMCDragonkai
Copy link
Member Author

Ok so now I'm trying to use a CJS module inside Jest.

It forces me to now do this:

import { default as AbstractError } from '@matrixai/errors';

This is true for both jest and tsx too.

It appears that once you are a ESM, and you try to import from a CJS package, you now have to pattern match the default, it's not automatically turned into the default import.

@CMCDragonkai
Copy link
Member Author

CMCDragonkai commented Aug 12, 2023

https://2ality.com/2019/04/nodejs-esm-impl.html#interoperability

Basically once you're in ESM, any time you use CJS modules, you have to use the default import and then pattern match/deconstruct it out.

import errors from '@matrixai/errors';
const { AbstractError } = errors;

This is because once you're ESM, you consider the entire exports object in CJS to be just the default export all the time.

That basically means if we have third party dependencies that are still using CJS, we have to always default import them, and then pattern match it out.

This is the case for tsx and jest.

@CMCDragonkai
Copy link
Member Author

Ok so there's another problem with having ESM using CJS modules. The types. If I'm exporting both a class and the type, I would often just import the class which would give me the type as well. Now if I'm doing pattern matching, I don't get the type. If I try to import the type again, that conflicts with the pattern matched name.

It seems then, it would really require a wholesale conversion of all packages then, cannot really start at js-db.

@CMCDragonkai
Copy link
Member Author

Tested cross-usage with MatrixAI/js-errors#13. It all worked fine.

@CMCDragonkai
Copy link
Member Author

MatrixAI/js-async-cancellable@e87f684 - already pushed to staging.

@CMCDragonkai
Copy link
Member Author

MatrixAI/js-resources@5708b7e pushed to staging.

@CMCDragonkai
Copy link
Member Author

Actually discovered an issue, the NODE_OPTIONS=--experimental-vm-modules won't work because it needs to set an env variable on windows. The usual way we deal with this is to create a custom script just for this. Will be easier than having an entire dependency like cross-env just to set variables.

@CMCDragonkai CMCDragonkai merged commit 4df2423 into staging Aug 12, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant