GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — [WIP] Migrate _everything_ to modules by weswigham · Pull Request #35561 · microsoft/TypeScript 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 Sheetal Nandi (@sheetalkamat), Andrew Casey (@amcasey), Matt Bierner (@mjbvz), Mine Starks (@minestarks) for you. Feel free to loop in other consumers/maintainers if necessary |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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 Sheetal Nandi (@sheetalkamat), Andrew Casey (@amcasey), and Mine Starks (@minestarks) so they are aware of the changes. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| @@ -1,484 +1,422 @@ | |||
| // Used by importFixes, getEditsForFileRename, and declaration emit to synthesize import module specifiers. | |||
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Wesley Wigham (@weswigham) looks like this comment got lost during "terraforming"
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
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 😀 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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 🙏 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| 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 an error while loading. Please reload this page.
There was a problem 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!
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
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. ❤️ |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Paul Draper (@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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Pretty helpful explainer! Thank you very much Wesley Wigham (@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? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Seems like the issue is fixed! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I have some worries about this. I'm using some API that is marked as |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Jack Works (@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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Superceded by jakebailey#1 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.