Module conversion tracking issue / notes

[jakebailey]

I'm using this thread to dump my brain and to note problems as I solve them (or to discuss).

Issues of note:

• Determining Module Migration and Shipping Strategy #49037
• Reduce typescript package size #27891
• Module conversion work #46744 (Eli's old notes)
• Migrate the TypeScript Project to Use Modules #35210
• Module conversion #46567

2022-05-31

Changes since the previous effort

• The "typeformer" (the code that does the transform) now lives at https://github.com/jakebailey/typeformer, and has been rewritten using ts-morph to preserve comments and formatting.
  • The old typeformer use TS's own transform and emitter, but the emitter is not careful enough to preform TS-to-TS transformations (I have a list of bugs I've discovered, which don't matter for JS emit)
• My WIP changes are semi-automatically pushed to a stack of PRs on my fork, starting here.
  • It's "best" to look at the difference between the "indent" commit and the last commit, e.g. this diff (but note that this link may not be up to date, is is as of now).
  • Please don't comment on these PRs; the script I run to create this stack is not very smart and will force-push if I insert a new commit in the middle. I wish I could just use gerrit, but...
• I have removed the esbuild step from the previous effort; not because we don't plan to use it, but because it's simplest to start by using raw modules and then tweaking the output later. If we're going to be modules, I'd hope we can be run through a bundler successfully.
• The faked namespace modules (barrel modules that at runtime behave very similarly to the old namespaces) have been moved to a dedicated folder in each project, rather than being dumped at the top level.
• Use webworker typings for webServer.ts #46944 has to be reverted; this code doesn't seem to compile properly when actually built.

typeformer bugs

• The typeformer's inlineImports step converts globalThis.assert into assert, because it thinks it's a global function that should be settable. Technically true, but if we wanted that, we probably wouldn't have written globalThis explicitly. This is just a bug when I rewrote the step.
• Sometimes #region comments are dropped. Likely a bug that can be worked around like other ts-morph oddities to do with comment preservation (which it is almost always good at, but sometimes not).
• Sometimes comments get duplicated, e.g. // DestructuringAssignmentType in src/compiler/types.ts.
• The output path is determined by the project name; this was changed in my iteration of the typeformer, but should be hand-tweaked back to the right path, e.g. the debug code (dbg.ts) that is dynamically loaded in, or loggedIO.

Unsolved problems

• My version of the typeformer's "inlineImports" pass is very aggressive, importing everything from their original declaration if possible. This is closest to how we would write the code by hand (if we had done so from the start), but the circularity of the codebase has broken things.
  • e.g., debug.ts imports semver.ts, but the Semver class creates a static property zero, which calls the Semver constructor, which then contains Debug.assert calls.
  • This only happened because the top of debug.ts contains import { Semver } from './semver, whereas before, the code didn't do this but instead referred to the namespace, so never actually caused that file to be evaluated.
  • It's totally possible that we'd hit this circularity issue today, except that most code is not at the top level and avoids the problem entirely, or has worked "magically" thanks to the order we happen to merge namespaces in.
  • We could just import these from the namespaces (like Module conversion #46567 does), which I think would fix this, but this is really unnatural code structure and auto-imports will not function properly.
• The System interface (aka ts.sys) is set (ts.sys = ...) in order to support the browser, but in the module version, modules do import { sys } from '...'), which makes it very difficult to do like this. What can do do here?
• Additionally, the System interface has getExecutingFilePath (among other functions), which only makes sense for TS packaged as a self contained file.

TODOs

• Figure out what to do about the API; the past effort talks about API extractor, which is the right thing to do, but we need to make sure that we produce an output that describes the API as namespaces, not as reexported modules. Re-export class from namespace #4529 makes this hard.
• We need to create the entrypoints for the new exported API; right now it just outputs as outDir, but I don't think we intend to just allow everyone to import our internals directly. outFile made a file that did the right thing before, and after we just need to write something like tsc.js which exports * from _namespaces/ts.ts or something.
  • See also exportAsModule, which hacks together a module.exports = ... line rather than emitting it via export.
• Deprecations were previously done via namespace merging (i.e. deprecating "Map" in favor of "ESMap"); these are very messy when not coming up with a single file.
• registerRefactor uses side effects of file loading to register refactorings. This technically works OK after modification if the fake namespace modules still import these files, but it'd be better to refactor this to work like the transforms (which are just imported in other files instead).
• Loads of type names are duplicated; this previously didn't matter because you'd just fully qualify names for another namespace, but now that things are directly imported, you end up with these random fully-qualified names which would be better done differently. For example, fakeHosts.ts implements certain interfaces, but does so with classes named the same as the interface. Maybe just do FakeSomeInterface?

Other stuff

• We should be able to hide the big refactor when it's merged using a .git-blame-ignore file. See:
  • https://github.com/microsoft/vscode/blob/main/.git-blame-ignore
  • https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view

[weswigham]

My version of the typeformer's "inlineImports" pass is very aggressive, importing everything from their original declaration if possible. This is closest to how we would write the code by hand (if we had done so from the start), but the circularity of the codebase has broken things.
e.g., debug.ts imports semver.ts, but the Semver class creates a static property zero, which calls the Semver constructor, which then contains Debug.assert calls.
This only happened because the top of debug.ts contains import { Semver } from './semver, whereas before, the code didn't do this but instead referred to the namespace, so never actually caused that file to be evaluated.
It's totally possible that we'd hit this circularity issue today, except that most code is not at the top level and avoids the problem entirely, or has worked "magically" thanks to the order we happen to merge namespaces in.
We could just import these from the namespaces (like #46567 does), which I think would fix this, but this is really unnatural code structure and auto-imports will not function properly.

I'd fixed this by hand a few times in my version - I usually swap deep imports for barrel file imports and reorder the barrel imports to match our tsconfig order, since that represents the canonical execution order we desire. Sometimes some barrel imports need to be replaced with deep imports to break circularities, but it wasn't too many, iirc.

This:

e.g., debug.ts imports semver.ts, but the Semver class creates a static property zero, which calls the Semver constructor, which then contains Debug.assert calls.

? I don't see a Semver reference in debug.ts locally, and semver comes after debug in our tsconfig, so semver's never available during debug's setup (not that it seems to need to be?).

or has worked "magically" thanks to the order we happen to merge namespaces in.

Nothing too magic about it. The tsconfig order is curated to be a correct execution order - so long as barrel files reproduce the same order (either automatically or by hand), we should be equivalent, at least.

The System interface (aka ts.sys) is set (ts.sys = ...) in order to support the browser, but in the module version, modules do import { sys } from '...'), which makes it very difficult to do like this. What can do do here?

We have a setSys function in sys already as part of my work years ago - we should swap external stuff over to using that if it isn't already.

Additionally, the System interface has getExecutingFilePath (among other functions), which only makes sense for TS packaged as a self contained file.

It still makes sense for a tree of files, imo - intuitively I feel like it should be the entrypoint file, or whatever was initially executed by node. (be it tsc.js or tsserver.js or bin/whatever)

[jakebailey]

? I don't see a Semver reference in debug.ts locally, and semver comes after debug in our tsconfig, so semver's never available during debug's setup (not that it seems to need to be?).

I meant to say Version, which is defined in semver.ts. It's referenced many times. (I was confusing it with the Semver package everyone else uses.)

I'd fixed this by hand a few times in my version - I usually swap deep imports for barrel file imports and reorder the barrel imports to match our tsconfig order, since that represents the canonical execution order we desire. Sometimes some barrel imports need to be replaced with deep imports to break circularities, but it wasn't too many, iirc.

Nothing too magic about it. The tsconfig order is curated to be a correct execution order - so long as barrel files reproduce the same order (either automatically or by hand), we should be equivalent, at least.

My overall goal was to end up with a codebase that felt like one that had been written with modules; this means eliminating these barrel imports from files. Otherwise, auto-imports will not function properly because auto-imports will prefer importing directly from the declaring modules. It also means not requiring that a given file import each file in a specific order either, given everyone (including our own import organizer) will not preserve this order.

Nothing too magic about it. The tsconfig order is curated to be a correct execution order - so long as barrel files reproduce the same order (either automatically or by hand), we should be equivalent, at least.

This is going to be rough to maintain in the long term, because I don't think the average person knows that this ordering has any impact. I sure didn't; I've never used a project that listed its files explicitly, only globs. I would hope that we can stop having so few files just to not have to add more, and instead break the code apart a bit. Especially if perf improves because we're not running every file in a closure where each access to the old namespace was actually fully-qualified under the hood.

We have a setSys function in sys already as part of my work years ago - we should swap external stuff over to using that if it isn't already.

My understanding had been that if one has a import { sys } from "..." that you're stuck with whatever value existed at import time, but searching it now, I guess export let ... is some sort of magical "live" view, which does fix this. That's good.

It still makes sense for a tree of files, imo - intuitively I feel like it should be the entrypoint file, or whatever was initially executed by node. (be it tsc.js or tsserver.js or bin/whatever)

Yep, this is more of a plumbing problem. I just wanted to note it so I could delete my random TODOs.

[weswigham]

This is going to be rough to maintain in the long term

I mean, it already is, it's one of the hidden costs if using namespaces. It just only comes up when someone finds they want to adjust the order (because, eg, they want to start using utilities somewhere new or something). Likewise, it'll only come up with modules when someone wants to import from a new module that's already in use elsewhere (and that module does static initialization), which, in practice, is pretty rare.

[weswigham]

this means eliminating these barrel imports from files

So this is a style argument, really. Imo, you're supposed to import from the barrel files for everything other than your immediately adjacent files (and we've talked about our autoimport behavior re: this before, and the only way we'll get better at it is honestly using it ourselves and seeing the upsides and downsides of each approach). Still, our first concern should probably be retaining a functional execution order (T.T) and not a specific style - that can come later.

[jakebailey]

My next step is to revert my changes that made imports really aggressive in favor of importing from the barrels, just to confirm that things run again (and to double check the state of auto-imports), so that should hopefully get me back to a working codebase.

I was really hopeful that we could write things like everyone else, but it's a shame about the execution ordering.

[jakebailey]

2022-08-29

I took a few months off of this to work on other issues, but have recently picked this back up again with the intent to make this breaking packaging change for 5.0 (it seems like a good idea to not do this on a minor release).

Last week, I hit a major milestone and got the test suite to run; all tests pass except the API tests (which don't pass because I haven't started on fixing the d.ts files yet).

• The entire build is just generateLibs and tsc -b ./src, which is a great simplification from our current state. I expect this to only expand for things like api-extractor and potentially a bundler if we still need to support a single-file for some outputs. The build time is roughly the same as main for a clean build of every project, but a good deal faster than main when making an incremental change to checker.ts.
• Getting tests running required a reordering of things in the namespaces, as well as moving some helper code around such that tests do not import other tests.
• The tests only work via the serial test runner, i.e. what's built into mocha. Our own custom parallel runner does not work. I am investigating this, however, mocha has a built-in parallel test runner. Our own was built before it didn't, so I'm going to see if we can drop our own special runner and just stick to upstream.
• I had to bring the module transform back to importing things from the faked namespaces (as noted in my previous comment); this is not my preference, but our code is currently too circular / execution order dependent to use direct imports.
  • It's likely that we can just start with these namespace imports first, then fix the circularities over time. There's a tool that the VS Code team used to analyze their codebase for these sorts of issues, and would likely work on our own once it uses imports.
• getExecutingFilePath is still replaced with a dummy implementation; reasonably I expect this API to change to something more like getTypeScriptLibRoot or similar such that the path to our lib folder can be provided rather than being deduced relative to tsc.js or something.

[jakebailey]

I posted the above early in the day; I did get the parallel test suite working. Mocha's parallel runner unfortunately can't be used with our repo in its current state as it expects individual test files, which it then runs in parallel (never any actual tests within a file in parallel).

We do our own thing and end up with effectively one file. Potentially, we can switch to something "more normal", somehow, but it's no longer a blocker. If we were using something like jest or ava, they could potentially run tests within a file in parallel, but that's a much larger change.

[jakebailey]

2022-08-31

I have the parallel test runner working; it came down to the fact that the entrypoint I was using for the test runner was the namespace file, which broke because runner.ts exported a mutable variable to another file, but runner.ts hadn't yet "returned" to the namespace object's require('./runner.ts') to add the binding to its own exports, so it appeared to be undefined. This wouldn't happen if we used direct imports, but making the entrypoint be runner.js was enough.

While working on this, I prototyped a Jest runner that could run our compiler tests instead of the relatively large bit of code we wrote to make Mocha work; I think that's a good idea for the future. I only tried it because I was frustrated trying to get our own parallel runner to work (so was looking for any alternative), but I'll defer that to another day now that I have all tests running besides the API tests.

Next up is api-extractor, and getting some proper entrypoints for things like tsc.js, tsserver.js, typescript.js, etc, which we should now be able to build unconditionally in build mode as they will be small.

[jakebailey]

2022-09-02

I have the entrypoints set up and working for the libraries/executables, and a basic api-extractor config for one of them. Unfortunately, things seem to fail on that front. I've reported microsoft/rushstack#3612. But, I've noticed that our d.ts output is a little suboptimal; the case that we are currently breaking on is an import that shouldn't even be emitted as it can be safely imported. I'm sure we've gotten someone reporting that issue before.

We'll have to eventually call api-extractor programatically; one transformation we will need to do again is to convert our const enum to enum; I doubt api-extractor does this for us.

[jakebailey]

But, I've noticed that our d.ts output is a little suboptimal; the case that we are currently breaking on is an import that shouldn't even be emitted as it can be safely imported. I'm sure we've gotten someone reporting that issue before.

This turned out to be part of the bug; hand-fixing these in the d.ts file allowed api-extractor to run to completion, although the output did not seem to exclude @internal declarations. I'll have to come up with some sort of reproducer for this.

[jakebailey]

2022-09-12

The aforementioned import() thing has to do with #44044 and #49730; the cases that crash api-extractor are ones where we don't emit an import statement because the original file didn't have one, so emit an import expression instead. api-extractor shouldn't be crashing for these cases, but I'm going to try and explicitly annotate the offending pieces of code and see how far I get.