MatrixAI / js-logger

TypeScript/JavaScript Logger Library
https://polykey.com
Apache License 2.0
1 stars 0 forks source link

feat: migrate to ESM #29

Closed CMCDragonkai closed 1 year ago

CMCDragonkai commented 1 year ago

Description

ESM native should be ready, and PKE sort of needs this to be the case. And we had a long-standing issue in https://github.com/MatrixAI/TypeScript-Demo-Lib/issues/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 https://github.com/microsoft/TypeScript/issues/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:

Issues Fixed

Tasks

Final checklist

ghost commented 1 year ago
👇 Click on the image for a new way to code review #### [![Review these changes using an interactive CodeSee Map](https://s3.us-east-2.amazonaws.com/maps.codesee.io/images/github/MatrixAI/js-logger/29/ba5f08be/44f39303f4fbad7688f5a077e36d20a1d444190e.svg)](https://app.codesee.io/r/reviews?pr=29&src=https%3A%2F%2Fgithub.com%2FMatrixAI%2Fjs-logger) #### Legend CodeSee Map legend
CMCDragonkai commented 1 year ago

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.

CMCDragonkai commented 1 year ago

Do note that this means:

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

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

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

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

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

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

CMCDragonkai commented 1 year ago

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

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

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

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

    "moduleResolution": "NodeNext",
    "module": "ESNext",
CMCDragonkai commented 1 year ago

It's also necessary for types to come first.

CMCDragonkai commented 1 year ago

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

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

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

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

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

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

CMCDragonkai commented 1 year ago

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

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

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

Tested cross-usage with https://github.com/MatrixAI/js-errors/pull/13. It all worked fine.

CMCDragonkai commented 1 year ago

https://github.com/MatrixAI/js-async-cancellable/commit/e87f684ddda3319ffb03a9da007131db9c25f38e - already pushed to staging.

CMCDragonkai commented 1 year ago

https://github.com/MatrixAI/js-resources/commit/5708b7e880004b0cd0c31335af77675628663343 pushed to staging.

CMCDragonkai commented 1 year ago

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.