-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
[WIP] Migrate _everything_ to modules #35561
Conversation
Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @amcasey, @mjbvz, @minestarks for you. Feel free to loop in other consumers/maintainers if necessary |
Thanks for the PR! It looks like you've changed 'preProcess.ts' in some way. Please ensure that any changes here don't break consumers with unique project systems such as Visual Studio. Pinging @sheetalkamat, @amcasey, and @minestarks so they are aware of the changes. |
@@ -1,484 +1,422 @@ | |||
// Used by importFixes, getEditsForFileRename, and declaration emit to synthesize import module specifiers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@weswigham looks like this comment got lost during "terraforming"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, looks like I should copy the comment ranges from the namespace declaration I remove onto the first declaration inside the namespace.
cfacc0f
to
987c8b9
Compare
987c8b9
to
44fcd2d
Compare
65cc165
to
00c1d9d
Compare
00c1d9d
to
802f955
Compare
This seems to be blocked on microsoft/rushstack#1029 so long as we want to use |
Is there a rationale for this change explained anywhere? It feels there is a chance it would set the compiler back in loading time and memory, as well as obvious risk of destabilisation due to many changes involved. The compatibility with older node or other hosts might worsen. Following trend is an upside, but quite a vague one. I hope I miss some obvious measurable upsides? |
I'm pretty sure this will not be affected, since this PR is not doing
If following coding standards by making real modules (and not using There are likely more benefits than the above, but one should not dismiss the benefit of code that is easier to learn and maintain. Future contributors will likely have an easier time to learn the code base after this change than before. Of course, I might be mistaken. I'll just let a TS member correct any impressions I got wrong 😀 |
While these points would well apply to a typical project, they don't practically make sense for TypeScript. Introducing a bundling step (which isn't there today) is a complex work, and there's virtually no dead code one could possibly shake. |
|
There is a bundling step today - the TS compiler itself just performs it (stupid concatenation of namespaces, plus a manually added Part of the reason we want to do this is because we have massive piles of dead code that could be eliminated in many uses. For example, the commandline compiler doesn't need any of the language server bits. |
Exactly what I meant, replacing a simple built-in option with a complex 3rd party piece. One is much more likely to generate support work than the other.
I'm sorry could you be a little more specific? It's a good convention for this repo for significant changes to be accompanied with at least a simple rationale. This here is one massive fundamental change. Way less involved changes are discussed in depth on merits, can at least an outline be here please? Specifically on the point of piles of dead code in I remember from the very early days But of course you've mentioned it's only a part of the reason for this change. It'd be fair to outline the key ones (maybe just edit the PR description?), to shut down all this unnecessary grumbling, wouldn't it? Many thanks 🙏 |
return stableSort<[number, string]>(result, (x, y) => compareValues(x[0], y[0])); | ||
} | ||
export function formatSyntaxKind(kind: SyntaxKind | undefined): string { | ||
return formatEnum(kind, (<any>ts).SyntaxKind, /*isFlags*/ false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi.
Not pretty sure, but I think (ts).SyntaxKind could not get the enum Object. (In my simple test case, it is undefined)
I use a new script to generate a new file like diagnosticMessages.generated.json to remove const keyword and get working enums.
I change it manually, and seems hard to use script to resolve this, so this comment is probably just a remind?
Hope this help!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We compile with preserveConstEnums
to keep them in the resulting file.
as people do code for bundlers today and not direct for the targeted environment. I like this a lot i am creating typescript-rollup a rollup based module resolver |
As the author of #27891...why? There's no compatibility reason for bundling...support for commonjs modules is assumed for any setup using npm packages. And extreme web-style compression isn't needed; npm authors don't minify or strip comments and READMEs and such from the package (or if they do, they included both versions). The size instantly drops by 4x just by not including multiple versions of the code, i.e. using reusable modules. In fact, I'm slightly perplexed that TS struggles with this; it feels as if the bundling process it currently does is harder than what ever other npm-published project has done which is use Node.js normally. |
I wrote this since Typescript is not only used in Node, it's also used in browsers by various online code editors (VSCode, stackblitz, codesandbox and others) and I assumed they wanted to keep similar support for this use case. Typescript has historically been bundled as single files for various purposes, I assumed they might still want bundles for e.g. Of course, it's possible to leave bundling of TS for web usage up to users. Thought I believe it's definitely a requirement to bundle TS since it's not really feasible to load thousands of files in a browsers. |
TypeScript sources live in multitude of files, and generate 3 key outputs: The source files can cross-reference each other without ceremony: This module migration here intends to have similar number of inputs/outputs, but:
Now there is an ongoing discussion about merits of specific approach of doing this migration, but no clarification for the elephant in the room. Why? What's the significant measureable upside? 1) extra tooling, 2) extra code ceremony, 3) major code churn -- each have massive costs versus current setup. It just feels surprising to see such significant bet with vague/non-existent rationale. |
The idea is that we can refactor our codebase to modules, generate the existing outputs, and then factor everything into smaller, self-contained dependencies that you can include a-la-cart - so while Plus, to directly counter each of your points:
Consumers of
Yes and no. That "ceremony" is implicitly being added by the compiler right now - all those cross-file references implicitly compile to
the "extensive non-local refactoring" is done by a tool at this point, so... 🤷 if the results are typechecked and tested, and the transform done by tools... does it matter? |
For context: we've been grumbling within the team that we should do this for literally years - since before my first internship on the team. It's simply a daunting task to migrate a codebase of this size. When I migrated |
While there's no question that JS modules are a clustercuss, consumers of npm packages are always using CommonJS-compatible tools. That's a core part of npm. For other hypothetical types of distributables, like a Web CDN or a Nashorn-powered JAR, the concern would be warranted, but npm packages universally assume CommonJS support, both for their own modules and their dependencies.
That's be great. In #27891 I mention the example of prettier (code formatter), which needs the TS parser but literally nothing else. But even without multiple npm packages, multiple modules will be a most welcome improvement. ❤️ |
@pauldraper i simply do my own builds of typescript and nashhorn is deprecated we now got graaljs it is easy to do own typescript builds from this messy codebase first you change or overwrite the tsconfig to throw out ESNext and then your near your goal feed it into rollup and it can be adjusted the results are amazing you do not even understand the build process that they use. i plan to automate that now with github actions that is then the final step i will publish it as typees i will put a link here when i am done at present i only did that for my personal use but i think now after 5+ years it is time for a maintained typescript fork. |
Pretty helpful explainer! Thank you very much @weswigham Effectively this is a first step towards an alternative package/bundling structure better suitable to tree shaking, makes a good sense. Reasonable question: is it viable instead of that to ONLY make the emit tree-shakeable? |
Seems like the issue is fixed! |
Oh my god, it's finally happening! @mihailik is raising an important point here:
When I started learning JS and node back somewhere in Node-v13 I started using ESM instead of commonjs after about one month, and I hardly ever used commonjs since then. When I started with TypeScript, the documentation and various "best practices" were already recommending to use ESModules instead of file-crossing namespaces and The TypeScript codebase is already pretty daunting on its own because of its size, complexity and popularity. It doesn't make it easier if you aren't hardened in the use of pre-esm code. I'm really excited for this. I dove into the TS code base a few times, but any attempts at hacking something always failed miserably. Experimenting in a foreign code base is often a lot easier when it's in ESM. More and more packages are being published with an additional ESM distribution, which makes exploring them or tracking down bugs an absolute charm, as the project-and code-shape remains clearly visible, and the comments are often not stripped. |
I have some worries about this. I'm using some API that is marked as |
@Jack-Works do not worry about that once this step is done and your more familar with the new sturcture you will find easy ways to workaround that often it can be done with less effort i for my self create a lot of dev bundels from my dependencies with custom own named exports. The Rollup tooling from today can easy handle all this scenarios. |
Superceded by jakebailey#1 |
The initial commit here is generated by executing https://github.com/weswigham/typeformer on our codebase. It can be regenerated as needed. That alone does not quite make this migration complete - with that done, the following needs doing to be able to adopt it (it is already editable and buildable):
gulp generate-diagnostics
to produce a module on each invocationwebpack
andapi-extractor
to reproducetypescript.js
,typescript.d.ts
,typescriptServices.js
,tsserverlibrary.js
, andtsserverlibrary.d.ts
from our modules.gulp LKG
to LKG all the new stuff (note: should changepackage.json
'smain
to point at a module entrypoint after first module LKG)gulp
tasks still function as expectedSee weswigham#50 for progress on the above - I only plan on merging it into this when this whole this is near merging, to keep the rebasing easy.