Reformat imports to be one identifier per line - #51565
Conversation
Sorry, something went wrong.
|
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.
| 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.
This file didn't get changed besides sorting because format-imports doens't seem to support this file type for some reason.
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
Wesley Wigham (weswigham)
left a comment
There was a problem hiding this comment.
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?
Sorry, something went wrong.
Yes, we can try that. The choice is mostly arbitrary, but I did it because that's what I'm familiar with from using eslint-plugin-simple-import-sort and format-imports, which both default to case insensitive. But, I am using eslint's built in sorter instead, so I can just flip it back to being sensitive. I do wonder if we'd have a better time switching to import type for type imports, though. Would sure make finding actual cycles easier. |
Sorry, something went wrong.
| 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.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
I prefer the more concise form if none of our tooling requires the more verbose form, too.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
|
Alright, I'm going to merge this; likely this will cause conflicts for existing PRs but hopefully fewer later. |
Sorry, something went wrong.
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.