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

Reformat imports to be one identifier per line #51565

Merged
merged 6 commits into from
Nov 17, 2022

Conversation

jakebailey
Copy link
Member

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.

@typescript-bot
Copy link
Collaborator

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

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Nov 16, 2022
@@ -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";
Copy link
Member Author

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.

@jakebailey
Copy link
Member Author

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.

Copy link
Member

@weswigham weswigham left a 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?

@jakebailey
Copy link
Member Author

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 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.

Comment on lines 71 to 72
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";
Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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.

@jakebailey jakebailey marked this pull request as ready for review November 17, 2022 00:22
@jakebailey
Copy link
Member Author

Alright, I'm going to merge this; likely this will cause conflicts for existing PRs but hopefully fewer later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants