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.

Sign up for GitHub

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

Jump to bottom

import multiline optimization #13353

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

import multiline optimization #13353

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9224b41
import multiline optimization
GlebDolzhikov Aug 24, 2022
d0b6f73
import multiline optimization
GlebDolzhikov Sep 1, 2022
4e72428
import multiline optimization
GlebDolzhikov Sep 1, 2022
b685b9b
import multiline optimization
GlebDolzhikov Sep 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 23 additions & 16 deletions src/language-js/print/module.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
"use strict";

const { isNonEmptyArray } = require("../../common/util.js");
const { isNonEmptyArray, getLast } = require("../../common/util.js");

const {
builders: { softline, group, indent, join, line, ifBreak, hardline },
} = require("../../document/index.js");
Expand Down Expand Up @@ -28,6 +29,7 @@ function printImportDeclaration(path, options, print) {
const node = path.getValue();
const semi = options.semi ? ";" : "";
/** @type{Doc[]} */

const parts = [];

const { importKind } = node;
Expand All @@ -38,14 +40,16 @@ function printImportDeclaration(path, options, print) {
parts.push(" ", importKind);
}

const groupId = Symbol("import");

parts.push(
printModuleSpecifiers(path, options, print),
printModuleSource(path, options, print),
printModuleSpecifiers(path, options, print, groupId),
printModuleSource(path, options, print, groupId),
printImportAssertions(path, options, print),
semi
);

return parts;
return group([parts]);
GlebDolzhikov marked this conversation as resolved.
Show resolved Hide resolved
GlebDolzhikov marked this conversation as resolved.
Show resolved Hide resolved
}

function printExportDeclaration(path, options, print) {
Expand Down Expand Up @@ -153,7 +157,7 @@ function shouldExportDeclarationPrintSemi(node, options) {
return false;
}

function printModuleSource(path, options, print) {
function printModuleSource(path, options, print, groupId) {
const node = path.getValue();

if (!node.source) {
Expand All @@ -163,14 +167,14 @@ function printModuleSource(path, options, print) {
/** @type{Doc[]} */
const parts = [];
if (!shouldNotPrintSpecifiers(node, options)) {
parts.push(" from");
parts.push(" ", indent(ifBreak("", softline, { groupId })), "from");
}
parts.push(" ", print("source"));

return parts;
}

function printModuleSpecifiers(path, options, print) {
function printModuleSpecifiers(path, options, print, groupId) {
const node = path.getValue();

if (shouldNotPrintSpecifiers(node, options)) {
Expand Down Expand Up @@ -220,16 +224,19 @@ function printModuleSpecifiers(path, options, print) {

if (canBreak) {
parts.push(
group([
"{",
indent([
group(
[
"{",
indent([
options.bracketSpacing ? line : softline,
join([",", line], groupedSpecifiers),
]),
ifBreak(shouldPrintComma(options) ? "," : ""),
options.bracketSpacing ? line : softline,
join([",", line], groupedSpecifiers),
]),
ifBreak(shouldPrintComma(options) ? "," : ""),
options.bracketSpacing ? line : softline,
"}",
])
"}",
],
{ id: groupId }
)
);
} else {
parts.push([
Expand Down
6 changes: 2 additions & 4 deletions tests/format/flow-repo/declare_export/__snapshots__/jsfmt.spec.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1363,10 +1363,8 @@ import { specifierNumber } from "ES6_Named1"; // Error: Did you mean \`specifier
///////////////////////////////////////////////////
// == Multi \`export *\` should combine exports == //
///////////////////////////////////////////////////
import {
numberValue1 as numberValue8,
numberValue2 as numberValue9,
} from "./ES6_ExportAllFromMulti";
import { numberValue1 as numberValue8, numberValue2 as numberValue9 }
from "./ES6_ExportAllFromMulti";

var at1: number = numberValue8;
var at2: string = numberValue8; // Error: number ~> string
Expand Down
6 changes: 2 additions & 4 deletions tests/format/flow-repo/es6modules/__snapshots__/jsfmt.spec.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1516,10 +1516,8 @@ import { specifierNumber } from "ES6_Named1"; // Error: Did you mean \`specifier
///////////////////////////////////////////////////
// == Multi \`export *\` should combine exports == //
///////////////////////////////////////////////////
import {
numberValue1 as numberValue8,
numberValue2 as numberValue9,
} from "./ES6_ExportAllFromMulti";
import { numberValue1 as numberValue8, numberValue2 as numberValue9 }
from "./ES6_ExportAllFromMulti";

Comment on lines +1519 to 1521
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm not sure I prefer this over the original 🤔

Even where there is no as, new formatting makes a bit harder to scan through names. AFAIU, the main benefit we should expect from the change is the reduction of lines of code. But what does it improve in DX? Existing approach optimizes for cognitive ease and clean git diff – that will be a shame to lose.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point about as. The current formatting seems to work better for such imports.

As for clean diffs, I think the proposed formatting might be an improvement in that regard. Currently, the diff isn't clean when switching between the single-line and multi-line formatting happens. With the proposed formatting, that switching is seemingly going to happen less often, and the diff for switching single-line⇄double-line is quite clean, definitely cleaner than single⇄multi.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just realized I'm used to diff viewers that highlight in-line diffs like this:
image
And you're probably talking about raw unhighlighted output of git diff. That makes me wonder how many people view diffs without in-line highlighting and why they would do that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But what does it improve in DX?

Less scrolling through something that is edited mostly by tooling (auto-import, linters for sorting imports and removing unused ones), not manually? I wish we had some statistics on how many developers edit imports manually. That'd put this proposal in a good perspective. I guess these people are already in minority, but that's only a guess.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I never need write code more than 200 lines. Split the file, if it's too long, that's what ES Module for.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

auto-import

I have these lines in my vscode settings

  "javascript.suggest.autoImports": false,
  "vetur.completion.autoImport": false,
  "volar.completion.autoImportComponent": false,

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have these lines in my vscode settings

I'm surprised. To me, this is like formatting code manually instead of using Prettier. The only problem I ever had with auto-import has been fixed in TS 4.8. Do you think there are many people who share you preferences in this regard?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are few, seach on github

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also didn't know that someone these days moves imports manually)

var at1: number = numberValue8;
var at2: string = numberValue8; // Error: number ~> string
Expand Down
8 changes: 2 additions & 6 deletions tests/format/flow-repo/export_type/__snapshots__/jsfmt.spec.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,12 +68,8 @@ var n: IFoo2 = {prop: 42}; // Error: {prop:number} ~> {prop:string}
=====================================output=====================================
/* @flow */

import type {
inlinedType1,
standaloneType1,
talias1,
talias3,
} from "./types_only";
import type { inlinedType1, standaloneType1, talias1, talias3 }
from "./types_only";

var a: inlinedType1 = 42;
var b: inlinedType1 = "asdf"; // Error: string ~> number
Expand Down
13 changes: 5 additions & 8 deletions tests/format/flow/import-type-specifier/__snapshots__/jsfmt.spec.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,11 @@ import { Foo, type Baz } from "../module";
import type {} from "foo";

import type { somethingSuperLongsomethingSuperLong } from "somethingSuperLongsomethingSuperLongsomethingSuperLong";
import type {
a,
somethingSuperLongsomethingSuperLong,
} from "somethingSuperLongsomethingSuperLongsomethingSuperLong";

import transformRouterContext, {
type TransformedContextRouter,
} from "../../helpers/transformRouterContext";
import type { a, somethingSuperLongsomethingSuperLong }
from "somethingSuperLongsomethingSuperLongsomethingSuperLong";

import transformRouterContext, { type TransformedContextRouter }
from "../../helpers/transformRouterContext";

================================================================================
`;
36 changes: 22 additions & 14 deletions tests/format/js/import/__snapshots__/jsfmt.spec.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ import {
} from '.';
import {fitsIn, oneLine} from '.';



import { GET_DATA, GET_DATA_START, GET_DATA_START2, GET_DATA_START3} from "./test-export";

=====================================output=====================================
import {
runTaskForChanged,
Expand All @@ -32,6 +36,9 @@ import {
} from ".";
import {fitsIn, oneLine} from ".";

import {GET_DATA, GET_DATA_START, GET_DATA_START2, GET_DATA_START3}
from "./test-export";

================================================================================
`;

Expand All @@ -53,6 +60,10 @@ import {
} from '.';
import {fitsIn, oneLine} from '.';



import { GET_DATA, GET_DATA_START, GET_DATA_START2, GET_DATA_START3} from "./test-export";

=====================================output=====================================
import {
runTaskForChanged,
Expand All @@ -66,6 +77,9 @@ import {
} from ".";
import { fitsIn, oneLine } from ".";

import { GET_DATA, GET_DATA_START, GET_DATA_START2, GET_DATA_START3 }
from "./test-export";

================================================================================
`;

Expand Down Expand Up @@ -322,13 +336,10 @@ import {a2, somethingSuperLongsomethingSuperLong3} from 'somethingSuperLongsomet
=====================================output=====================================
import somethingSuperLongsomethingSuperLong from "somethingSuperLongsomethingSuperLongsomethingSuperLong";
import {somethingSuperLongsomethingSuperLong1} from "somethingSuperLongsomethingSuperLongsomethingSuperLong";
import a, {
somethingSuperLongsomethingSuperLong2,
} from "somethingSuperLongsomethingSuperLongsomethingSuperLong";
import {
a2,
somethingSuperLongsomethingSuperLong3,
} from "somethingSuperLongsomethingSuperLongsomethingSuperLong";
import a, {somethingSuperLongsomethingSuperLong2}
from "somethingSuperLongsomethingSuperLongsomethingSuperLong";
import {a2, somethingSuperLongsomethingSuperLong3}
from "somethingSuperLongsomethingSuperLongsomethingSuperLong";

================================================================================
`;
Expand All @@ -347,13 +358,10 @@ import {a2, somethingSuperLongsomethingSuperLong3} from 'somethingSuperLongsomet
=====================================output=====================================
import somethingSuperLongsomethingSuperLong from "somethingSuperLongsomethingSuperLongsomethingSuperLong";
import { somethingSuperLongsomethingSuperLong1 } from "somethingSuperLongsomethingSuperLongsomethingSuperLong";
import a, {
somethingSuperLongsomethingSuperLong2,
} from "somethingSuperLongsomethingSuperLongsomethingSuperLong";
import {
a2,
somethingSuperLongsomethingSuperLong3,
} from "somethingSuperLongsomethingSuperLongsomethingSuperLong";
import a, { somethingSuperLongsomethingSuperLong2 }
from "somethingSuperLongsomethingSuperLongsomethingSuperLong";
import { a2, somethingSuperLongsomethingSuperLong3 }
from "somethingSuperLongsomethingSuperLongsomethingSuperLong";

================================================================================
`;
Expand Down
4 changes: 4 additions & 0 deletions tests/format/js/import/brackets.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,7 @@ import {
soWeCanGetItTo80Columns
} from '.';
import {fitsIn, oneLine} from '.';



import { GET_DATA, GET_DATA_START, GET_DATA_START2, GET_DATA_START3} from "./test-export";