-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Reformat imports to be one identifier per line #51565
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
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 |
@@ -6,9 +6,9 @@ import { task } from "hereby"; | |||
import _glob from "glob"; | |||
import util from "util"; | |||
import chalk from "chalk"; | |||
import { exec, readJson, getDiffTool, getDirSize, memoize, needsUpdate, Debouncer, Deferred } from "./scripts/build/utils.mjs"; |
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.
This file didn't get changed besides sorting because format-imports
doens't seem to support this file type for some reason.
Without a formatter to handle this, I don't know that we can actually maintain this formatting; it might be the case that removing imports to a single import and then readding them makes it all one line. This is something in auto-import I haven't really tested. |
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.
Q: Can we try a case-sensitive sort? If we do so, it keeps types next to one another and functions next to one another in the import list, which I think'll be a bit easier to read diffs on. Or is there reason for preferring the case-insensitive sort?
Yes, we can try that. The choice is mostly arbitrary, but I did it because that's what I'm familiar with from using I do wonder if we'd have a better time switching to |
src/compiler/_namespaces/ts.ts
Outdated
import * as moduleSpecifiers from "./ts.moduleSpecifiers"; | ||
export { moduleSpecifiers }; | ||
import * as performance from "./ts.performance"; | ||
export { performance }; | ||
export * as moduleSpecifiers from "./ts.moduleSpecifiers"; | ||
export * as performance from "./ts.performance"; |
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.
Pretty much all import linters want to reformat the old code to split imports from exports. I had kept the old form because tooling (api-extractor, when I was using it, some bundlers) seemed to do a better job with it, but I swapped it to the more concice form in this PR so that reformatting imports works better. Maybe this is a bad idea and I should just undo these changes by hand.
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.
I prefer the more concise form if none of our tooling requires the more verbose form, too.
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.
I agree, the concise form is better.
I did a diff to see, and curiously this does actually seem to cause an output difference in esbuild. When we use this short, it's unable to remove some of the export objects, leaving things like:
// src/jsTyping/_namespaces/ts.server.ts
var ts_server_exports = {};
__export(ts_server_exports, {
ActionInvalidate: () => ActionInvalidate,
ActionPackageInstalled: () => ActionPackageInstalled,
ActionSet: () => ActionSet,
Arguments: () => Arguments,
EventBeginInstallTypes: () => EventBeginInstallTypes,
EventEndInstallTypes: () => EventEndInstallTypes,
EventInitializationFailed: () => EventInitializationFailed,
EventTypesRegistry: () => EventTypesRegistry,
findArgument: () => findArgument,
hasArgument: () => hasArgument,
nowString: () => nowString
});
Even though it's unreferenced. It's not a correctness problem, but is less efficient.
This made me remember that I did notice this in the process of the module conversion and that's why I left it as the less concise form. I was never able to minimize it.
I will probably revert these as to not regress our output and try and report it upstream.
Alright, I'm going to merge this; likely this will cause conflicts for existing PRs but hopefully fewer later. |
In a design meeting before modules, we decided that we would start by "doing nothing" to solve our import sorting problem, leaving them as roughly 120-wide blocks and just let auto-import do whatever.
That unfortunately has caused merge conflicts to happen, and while we knew that would happen, it seems to be worse than expected.
So, let's try doing one import per line.
Unfortunately, there is no eslint rule that would allow us to do this; the best we can do is to sort the imports once they are there, but that won't reformat them to match this format. All of the lint rules defer to prettier/dprint/etc, and we still haven't come to a conclusion on that front, as much as I would like that to be done and done.