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

Module conversion work #46744

Closed
7 of 19 tasks
elibarzilay opened this issue Nov 9, 2021 · 3 comments
Closed
7 of 19 tasks

Module conversion work #46744

elibarzilay opened this issue Nov 9, 2021 · 3 comments
Assignees
Labels
Discussion Issues which may not have code impact

Comments

@elibarzilay
Copy link
Contributor

elibarzilay commented Nov 9, 2021

This issue is to keep track of the module conversion work, which is ongoing at #46567:

  • Update the old conversion script to working state.
    Done, keep track of the code in a new fork to working state.
  • Script the generation so it can be continuously rebased on the tip: done in the run-transform script.
  • Make the tests run, similar to 1c3944343ee8 in Wesley's PR: included in the current PR, and generated via the script.
  • Fix treatment of newlines, mainly a bug that changes CRLF to LF if running on linux.
  • Make the diff more readable
    • Pre-script that unindents the code in a separate commit, which avoids a huge mass of mostly no-op changes.
    • Drop the many empty-line removals from the conversion diff. Done by another script that filters the resulting patch before applying it.
    • Possibly re-implement the conversion using ts-morph to make it more well-behaved wrt unnecessary changes (adding/removing newlines, still some comments get dropped, minor issues like changing ,s to ;s or dropping a suffix , in argument lists).
  • Bundle results
    • Decide on a bundler: most likely esbuild, since it fast enough to make multiple bundles (eg, for testing and for distributing) possible.
    • Need to decide how to use it:
      • Can run on the generated JS: will probably be sensitive to the generated format.
        Main problem: the modules are expressed indirectly which is more likely to cause problems.
      • A possibly better option is to run directly from the TS sources. This has a big issue of not dog-fooding tsc, but that could possibly still be used for testing (but see below).
    • Should also re-do the src/tsserverlibrary and src/typescriptServices stubs.
    • Some other files that need to be sorted out:
      • src/instrumenter (including src/loggedIO/tsconfig-tsc-instrumented.json)
      • src/services/exportAsModule.ts
  • Running the tests takes around 1.5x more time with the modules.
    • This might be resolved if running from a bundle, but if this and the actual bundles are done with esbuild then there's no dogfooding at all.
  • There are a few tests that still fail (12) -- should be resolved when there are bundles.
  • Make the parallel tests work. (This depends on the resolution of the above, since it's an issue with how the tests are implemented wrt the global describe binding.)
  • API extractor.
  • Make the linter happy.
  • Possibly a cleanup step, like getting rid of re-exporting modules
    for direct imports, and related (like *Impl.ts files).
@ahejlsberg
Copy link
Member

Running esbuild directly on the TS sources likely isn't an option because const enum inlining isn't implemented by esbuild (or at least isn't implemented for references across modules). See here.

@andrewbranch andrewbranch added the Discussion Issues which may not have code impact label Nov 9, 2021
@andrewbranch
Copy link
Member

That shouldn’t be a problem for local testing since const enum usage will still work without inlining via preserveConstEnums. For our final distributed bundle and perhaps for testing in CI, I think we want to use tsc as much as possible. But if using esbuild locally could let us run a test with near-zero delay, I think that would be awesome.

elibarzilay added a commit to elibarzilay/TypeScript that referenced this issue Dec 2, 2021
When the binding is itself one that was created by `__createBinding`,
re-use its descriptor, which avoids piling multiple levels of getters in
the case of multiple levels of exports.

Also related to microsoft#46744 and to microsoft/tslib#165.
elibarzilay added a commit to elibarzilay/TypeScript that referenced this issue Jan 10, 2022
When the binding is itself one that was created by `__createBinding`,
re-use its descriptor, which avoids piling multiple levels of getters in
the case of multiple levels of exports.

In addition, reuse a descriptor if the bindings is marked as
non-writable and non-configurable, which makes a getter not
necessary.  (This can be done manually if needed, even though tsc
doesn't do it now.)

Also related to microsoft#46744 and to microsoft/tslib#165.
elibarzilay added a commit to elibarzilay/TypeScript that referenced this issue Jan 11, 2022
When the binding is itself one that was created by `__createBinding`,
re-use its descriptor, which avoids piling multiple levels of getters in
the case of multiple levels of exports.

In addition, reuse a descriptor if the bindings is marked as
non-writable and non-configurable, which makes a getter not
necessary.  (This can be done manually if needed, even though tsc
doesn't do it now.)

Also related to microsoft#46744 and to microsoft/tslib#165.
elibarzilay added a commit to elibarzilay/TypeScript that referenced this issue Jan 12, 2022
When the binding is itself one that was created by `__createBinding`,
re-use its descriptor, which avoids piling multiple levels of getters in
the case of multiple levels of exports.

In addition, reuse a descriptor if the bindings is marked as
non-writable and non-configurable, which makes a getter not
necessary.  (This can be done manually if needed, even though tsc
doesn't do it now.)

Also related to microsoft#46744 and to microsoft/tslib#165.
elibarzilay added a commit that referenced this issue Jan 13, 2022
When the binding is itself one that was created by `__createBinding`,
re-use its descriptor, which avoids piling multiple levels of getters in
the case of multiple levels of exports.

In addition, reuse a descriptor if the bindings is marked as
non-writable and non-configurable, which makes a getter not
necessary.  (This can be done manually if needed, even though tsc
doesn't do it now.)

Also related to #46744 and to microsoft/tslib#165.
@jakebailey jakebailey self-assigned this Feb 7, 2022
@jakebailey
Copy link
Member

Closing in favor of #49332 (where I'll do something similar to this issue).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Issues which may not have code impact
Projects
None yet
Development

No branches or pull requests

4 participants