MatrixAI / TypeScript-Demo-Lib

TypeScript Library Demo Project using Nix https://matrix.ai
Apache License 2.0
3 stars 0 forks source link

Migrate to ESM (and get dynamic await) #32

Open CMCDragonkai opened 2 years ago

CMCDragonkai commented 2 years ago

What is your research hypothesis/question?

There is a conjunction of 4 features:

That enables some nifty capabilities:

These features are all coming online in various forms, however some initial experiments show that they involve alot of changes.

Review existing ideas, literature and prior work

  1. motivation from nativescript https://github.com/MatrixAI/js-polykey/issues/155#issuecomment-935348551
  2. experiment in js-id https://github.com/MatrixAI/js-id/issues/1#issuecomment-936105951
  3. ts-node support https://github.com/TypeStrong/ts-node/issues/1007
  4. Wait for TS 4.5 release

Research conclusion

See the notes in https://github.com/MatrixAI/js-logger/issues/29

CMCDragonkai commented 2 years ago

It would be nice if changing to this didn't involve changing all of our import paths.

CMCDragonkai commented 2 years ago

Solving this will enable https://github.com/MatrixAI/js-id/issues/2

CMCDragonkai commented 2 years ago

Pkg doesn't currently support ES modules. But it should soon: https://github.com/vercel/pkg/issues/782.

Currently it may require us to use esbuild.

The esbuild could be trialed out to see if it's better than using webpack which we had removed before, and see if it can help also deal with non-transpiled files that are in ./src and automatically move to ./dist.

It may also help with doing some optimisation work like tree-shaking. But of course it doesn't make sense to put it all into one single file like we were doing in webpack. Library modules should be able to imported separately.

CMCDragonkai commented 2 years ago

Note that dynamic imports already work. Right now they are just translated to require calls. However being able to use these for cross-platform implementation swapping will require top level awaits.

CMCDragonkai commented 2 years ago

With #37 merged, we are one step closer.

Before working with ES modules and dynamic imports that use ES modules, it must be tested that it works in vercel pkg. Keep track of this issue: https://github.com/vercel/pkg/issues/1603

Also important that it works with ts-node too.

CMCDragonkai commented 2 years ago

Experimental support for ESM in ts-node is now available: https://github.com/TypeStrong/ts-node/issues/1007#issuecomment-1139917958

CMCDragonkai commented 2 years ago

Moving to ESM is a big task, there are other tools we need support for: https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c

CMCDragonkai commented 1 year ago

This would be necessary to support usage of WASM libraries: https://github.com/MatrixAI/Polykey/issues/422#issuecomment-1264827733

We also have to decide whether to use the .js import extension, or do something experimental to avoid having to always choose the extension to import it (and force extensionless imports).

CMCDragonkai commented 1 year ago

PKE is going to be on ESM. If we move the rest of PK ecosystem to ESM, we have to deal with:

  1. tsconfig-paths not working with ESM so we can't use ts-node to load things that use the alias paths
  2. possibly the fact that CJS cannot import/require things that are written in ESM, so we just publish ESM and hope for the best? Not sure.
CMCDragonkai commented 1 year ago

Note that:

What is doable is https://github.com/vercel/pkg/issues/1291#issuecomment-1518694260 - which uses a bundler to convert ESM to CJS, then uses pkg on CJS.

Furthermore any bundler to convert ESM to CJS does not support top-level await. Which means you often need refactor any usages of top level await like in this library https://github.com/Offroaders123/NBTify/issues/30#issuecomment-1600366281.

This is quite annoying, other bundlers like webpack focus on more a web-like environment and so has features that isn't really needed.

Node is coming out with its own single file executables too, but currently only supports CJS.

Deno is probably the only one that cleanly creates a single file executable with full ESM support but involves bunch of migration that can be difficult to do with all the existing assumptions.

CMCDragonkai commented 1 year ago

Should keep track of this project too https://github.com/nodejs/single-executable and https://dev.to/chad_r_stewart/compile-a-single-executable-from-your-node-app-with-nodejs-20-and-esbuild-210j

CMCDragonkai commented 1 year ago

If top-level await is a necessary construct, then we will not be able to use esbuild as the bundler. If so, then we should go with webpack since it's well understood, and I've used it quite a bit. Suppose our entire codebase was ES module native. Then the webpack would eliminate all ES modules and turn it into a single file. There will still need to be a virtual filesystem to deal with all the shared objects and other data files, but this could be sorted some how with deeper understanding of pkg.

Subsequently we can then use pkg as a lower level tool that consumes the output of webpack which just basically puts it all into a single executable file.

CMCDragonkai commented 1 year ago

This issue has become more important since PKE has gone ES native and right now importing our CJS modules is kind of broken. https://gitlab.com/MatrixAI/Engineering/Polykey/Polykey-Enterprise/-/merge_requests/2#note_1472206322

CMCDragonkai commented 1 year ago

ESM support will be in js-async-cancellable, js-resources already and will be merged to staging and master soon.

I was waiting on them first before pushing js-logger and js-errors. But I think these will be passed too.

All of this means all our packages will be ESM native by default.

It's still possible to use regular CJS packages but you have to pattern match off from a default import.

The true test is going to be js-db where all the relevant packages are updated to ESM and then see how it goes.

After the main changes are good, I believe that can just be replicated. @amydevs can you start helping out on the other packages following js-errors changes.

CMCDragonkai commented 1 year ago

First released package that is fully ESM: @matrixai/async-cancellable at 2.0.0.

CMCDragonkai commented 1 year ago

Also @matrixai/resources at 2.0.0.

However there's an error with js-errors strange.

CMCDragonkai commented 1 year ago

Ok had to regenerate the package-lock.json by first deleting the node_modules directory. I think this will fix it, and this will be released as 2.0.1.

CMCDragonkai commented 1 year ago

The js-errors is now done.

On to js-timer.

This has a problem.

It turns out having a file inside tests that imports another file locally:

import { sleep } from './utils.js';

Fails with jest. Jest's resolver cannot find it. Cannot find module './utils.js' from 'tests/Timer.test.ts'.

This does in fact require the moduleNameMapper.

Ok anyway, @matrixai/logger will have 4.0.0. And @matrixai/timer will have 2.0.0.

CMCDragonkai commented 1 year ago

To do:

Note that certain kinds of mocking is different and some WASM loading might be different too.

This will prove the ability to work with decorators and native modules (and worker threads).

If this works, an upgrade for all PK is possible.

CMCDragonkai commented 1 year ago

Note that top-level await does work in esbuild now. It was a later comment. Especially since tsx uses esbuild it would have to support top-level await.

Should also investigate https://github.com/privatenumber/pkgroll for bundling as it uses esbuild too. This will be important for the pkg optimisation for Polykey-CLI.

CMCDragonkai commented 1 year ago

So the new esm packages which I've released for a few upstream dependencies can lead to a better bundling situation for PK CLI Atm we use PKG to bundle all the code and also put together the executable. But it's a bad bundler The migration to esm took a bunch of work in weird places - tsx replaces ts-node, jest required a special node option, a bunch of stuff in tsconfig and package.json But one thing it revealed is that esbuild is back to being a possibility for bundling One major improvement that was unexpected is that benches and tests now run against compiled code. Meaning test results and benchmarking is far more accurate and would avoid regressions resulting from compilation bugs or mistakes There are 2 issues that is yet to be tested. ESM when dealing with native modules - the .node binaries, ESM when dealing with our decorators (due to esbuild). Esbuild could choke on some special features in TS if we are using them. Not sure. Top level await is going to be really useful for platform conditional exports Additionally we still use tsc for compilation which produces regular JS. Swc is now only used during testing. Esbuild is used during tsx execution. This can all lead to weird behaviour which is unfortunate. Hopefully in the future jest will use swc, tsx or something similar uses swc, and compilation uses swc... To maintain consistency. But for now the stitching works. (Funny how there are literally 3 different TS compilers being used in every project) But the fact that tsc is used for the final compilation means that when using esbuild for bundling, esbuild doesn't do any TS compilation. This keeps it simple. So we should see a far more lightweight executable with even compressed JS/minified JS inside the executable. The final step of combining it with a node executable is then sort of simple. Additionally a virtual filesystem is provided by PKG to enable loading of shared objects. These 2 things we could potentially replace with our own executable bundling system. The end result should be something that is far more secure, simpler, more customisable, and lighterweight Also I believe if we stick with PKG, esbuild would have to produce a cjs bundle at the end

CMCDragonkai commented 1 year ago

Small blocker on https://github.com/MatrixAI/js-workers/pull/12. The threadsjs types aren't properly exported and we are making use of 3 types.

import type { ModuleThread } from 'threads';
import type { ModuleMethods } from 'threads/dist/types/master';
import type { QueuedTask } from 'threads/dist/master/pool-types';

Currently none of the types work because they aren't properly exported by the upstream package.

Additionally a recent commit made QueuedTask exported, but again can't access it. It's also because by using ESM and also using the "exports" key, we can't just go straight to the dist directory to get it.

CMCDragonkai commented 1 year ago

Can confirm that decorator usage is fine.

CMCDragonkai commented 1 year ago

The js-workers is a problem:

  1. threads.js needs to be replaced because its getting unmaintained
  2. threadjs when using .ts files forces the usage of ts-node

This is blocking js-db migration since it has tests that rely on it.

We can try swapping out threadsjs for this https://github.com/piscinajs/piscina.

Maybe that can enable it, and we can then proceed with js-db.

It's possible we may have to change the way we create workers a bit.

CMCDragonkai commented 12 months ago

Due to https://github.com/jestjs/jest/issues/2549, vm isolation can introduce problems when testing and expecting errors to throw or certain types to be instances of something. Basically the globals inside nodejs isn't the same as globals in jest. Thus toThrow(TypeError) or toBeInstanceOf(Array) might fail if those objects are coming out of nodejs.

This doesn't just affect tests, but it could also affect internal code that has to do instanceof Array.

There's no good solutions, the workaround on tests is to do toHaveProperty('name', 'RangeError') instead of toThrow(RangeError). And instead of instanceof tests you'll need to find other ways of checking like Array.isArray instead of instanceof Array.

The good thing is that most of the native globals that come out of nodejs should have checking functions like Array.isArray.

And exceptions wise... we usually wrap it in our own error tree.

amydevs commented 12 months ago

I've been using toHaveProperty in js-ws so far

CMCDragonkai commented 3 weeks ago

@amydevs let's make the top level issue for ESM migration into Polykey Core Library. We could also create a Project for it - as it will end up grouping multiple ESM migration related issues.

CMCDragonkai commented 3 weeks ago

Convert this to a project, and then group all relevant ESM issues under this for all repos.

Make sure to link the background context of why you're patching the jest fastcheck problem, as well as then notes on how we are going to deal with the mocking issue - probably by not mocking at all where we can.

CMCDragonkai commented 2 weeks ago

Due to jestjs/jest#2549, vm isolation can introduce problems when testing and expecting errors to throw or certain types to be instances of something. Basically the globals inside nodejs isn't the same as globals in jest. Thus toThrow(TypeError) or toBeInstanceOf(Array) might fail if those objects are coming out of nodejs.

This doesn't just affect tests, but it could also affect internal code that has to do instanceof Array.

There's no good solutions, the workaround on tests is to do toHaveProperty('name', 'RangeError') instead of toThrow(RangeError). And instead of instanceof tests you'll need to find other ways of checking like Array.isArray instead of instanceof Array.

The good thing is that most of the native globals that come out of nodejs should have checking functions like Array.isArray.

And exceptions wise... we usually wrap it in our own error tree.

@tegefaulkes mentions that in https://github.com/MatrixAI/Polykey-CLI/issues/257, there is issues with using instanceof is a problem for any situation where the npm dependencies may be duplicated when using npm link. I still don't see any documentation on this concept called "npm modules". So as far as I can tell, fixing this means eliminating any common dependencies entirely from the package.json.

However in relation to ESM migration, relying on things like instanceof is flaky in our tests. And relying on jest and fast-check has a problem with https://github.com/dubzzz/fast-check/issues/4986.

Either way, it means for the benefit of testing, and fast-checking, we should avoid using instanceof in our tests, replacing it with toHaveProperty or preferably X.isX sort of functions that's native to nodejs. Additionally we have a patch workaround for the fast check bridge to jest here https://github.com/dubzzz/fast-check/issues/4986#issuecomment-2285225681. However we can also just avoid using the bridge, and just use fast-check directly within the test functions.

There are workarounds mentioned like using jest-environment-node-single-context as the testEnvironment but it's not ideal as it introduces other problems.

So...

CMCDragonkai commented 14 hours ago

@amydevs get your js-ws issue here too. And identify what is blocking here.