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

chore(linting) apply a rule that fixes import vs import type when is possible #8907

Merged
merged 10 commits into from
May 11, 2023

Conversation

asturur
Copy link
Member

@asturur asturur commented May 9, 2023

Motivation

This adds import consistency and probably speeds up aware bundler/parsers
The changes are all made with the lint --fix command and no human intervention.

Description

Changes

import type will be enforced when possible

Gist

In Action

@github-actions
Copy link
Contributor

github-actions bot commented May 9, 2023

Build Stats

file / KB (diff) bundled minified
fabric 902.416 (0) 304.561 (0)

@github-actions
Copy link
Contributor

github-actions bot commented May 9, 2023

Coverage after merging add-rule-import-type into master will be

83.67%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
index.node.ts7.69%100%0%14.29%17, 20, 23, 29, 32, 35
src
   ClassRegistry.ts100%100%100%100%
   Collection.ts94.71%94.64%86.67%97.09%101, 104, 207–208, 233–234
   CommonMethods.ts96.55%87.50%100%100%10
   Intersection.ts100%100%100%100%
   Observable.ts87.23%85.29%84.62%89.36%144–145, 170–171, 39–40, 48, 57, 91, 99
   Point.ts100%100%100%100%
   Shadow.ts98.36%95.65%100%100%178
   cache.ts97.06%90%100%100%57
   config.ts75%66.67%66.67%82.76%130, 138, 138, 138, 138, 138–140, 151–153
   constants.ts100%100%100%100%
   typedefs.ts100%100%100%100%
src/Pattern
   Pattern.ts92.21%91.89%90%93.33%116, 127, 136, 29, 92
src/brushes
   BaseBrush.ts100%100%100%100%
   CircleBrush.ts0%0%0%0%100, 102–104, 113, 113, 113, 115, 117, 119–121, 123–126, 133–134, 141, 143, 28–29, 37–41, 45–49, 56–59, 67–71, 73, 81, 81, 81, 81, 81–82, 84, 84, 84–87, 89, 97–98
   PatternBrush.ts97.06%87.50%100%100%21
   PencilBrush.ts91.01%82.35%100%93.75%122–123, 152, 152–154, 176, 176, 276, 280, 285–286, 68–69, 84–85
   SprayBrush.ts0%0%0%0%100–101, 103–104, 112, 112, 112, 112, 112–113, 115–116, 123–124, 126, 128–132, 141, 145–146, 146, 154, 154, 154–157, 159–162, 166–167, 169, 171–174, 177, 184–185, 187, 189–190, 192, 199–200, 202–203, 206, 206, 213, 213, 217, 22–23, 25–27, 27, 27–29, 33, 42, 49, 56, 63, 70, 89–91, 99
src/canvas
   Canvas.ts78.87%77.54%81.67%79.41%1001–1002, 1002, 1002–1004, 1006–1007, 1007, 1007, 1009, 1017, 1017, 1017–1019, 1019, 1019, 1025–1026, 1034–1035, 1035, 1035–1036, 1041, 1043, 1074–1076, 1079–1080, 1084–1085, 1198–1200, 1203–1204, 1277, 1396, 1519, 1589, 162, 187, 297–298, 301–305, 310, 333–334, 339–344, 364, 364, 364–365, 365, 365–366, 37, 374, 379–380, 380, 380–381, 383, 392, 398–399, 399, 399, 41, 442, 450, 454, 454, 454–455, 457, 539–540, 540, 540–541, 547, 547, 547–549, 569, 571, 571, 571–572, 572, 572, 575, 575, 575–576, 579, 588–589, 591–592, 594, 594–595, 597–598, 610–611, 611, 611–612, 614–619, 625, 632, 669, 669, 669, 671, 673–678, 684, 690, 690, 690–691, 693, 696, 701, 714, 742, 742, 800–801, 801, 801–802, 804, 807–808, 808, 808–809, 811–812, 815, 815–817, 820–821, 891, 903, 910, 931, 963, 984–985
   SelectableCanvas.ts94.39%91.16%94.55%96.63%1119, 1119–1120, 1123, 1143, 1143, 1201, 1254–1255, 1276, 1284, 1409, 1411, 1413–1414, 518, 698–699, 701–702, 702, 702, 751–752, 813–814, 867–869, 901, 906–907, 934–935
   StaticCanvas.ts96.85%92.86%100%98.61%1114–1115, 1115, 1115–1116, 1236, 1246, 1300–1301, 1304, 1339–1340, 1417, 1426, 1426, 1430, 1430, 1477–1478, 322–323, 340, 771, 783–784
   TextEditingManager.ts100%100%100%100%
src/color
   Color.ts92.16%86.49%100%94.29%330–331, 335–336, 339–340, 58, 88–89, 89, 91, 91, 91–92, 94–95
   color_map.ts100%100%100%100%
   constants.ts100%100%100%100%
   util.ts100%100%100%100%
src/controls
   Control.ts93.90%88.89%90.91%97.73%232, 319, 319, 354
   changeWidth.ts100%100%100%100%
   commonControls.ts100%100%100%100%
   controlRendering.ts81.63%78%100%84.78%106, 111, 121, 121, 45, 50, 61, 61, 65–72, 81–82
   drag.ts100%100%100%100%
   polyControl.ts5.97%0%0%11.11%100, 105, 119, 119, 119, 119, 119, 121–124, 124, 127, 134, 17, 25–29, 29, 29, 29, 29, 29, 29, 29, 50–56, 56, 56, 56, 56, 58, 63–64, 66, 76, 82–83, 83, 83–84, 88–90, 90, 90, 90, 90, 92
   rotate.ts19.57%12.50%50%21.43%41, 45, 51, 51, 51–52, 55–57, 59, 59, 59, 59, 59–61, 61, 61–63, 65, 65, 65–67, 67, 67–68, 73, 73, 73–74, 76, 78, 80–81
   scale.ts93.57%92.94%100%93.67%129–130, 132–134, 148–149, 181–183, 42
   scaleSkew.ts78.79%64.29%100%85.71%27, 29, 29, 29, 31, 33, 35
   skew.ts91.03%79.31%100%97.67%130–131, 162–163, 170, 176, 178
   util.ts100%100%100%100%
   wrapWithFireEvent.ts100%100%100%100%
   wrapWithFixedAnchor.ts100%100%100%100%
src/env
   browser.ts84.21%77.78%50%100%14, 17
   index.ts100%100%100%100%
   node.ts73.08%33.33%66.67%88.24%27, 31–32, 32, 32, 43
src/filters
   BaseFilter.ts21.93%22.81%32%19.05%106–109, 109, 109–110, 116, 116, 116–119, 137, 155, 169–174, 178–179, 179, 179–182, 182, 182, 182, 182–184,

@asturur
Copy link
Member Author

asturur commented May 9, 2023

There is also an export rule but is unclear how to use it

@asturur asturur requested a review from ShaMan123 May 9, 2023 21:02
@asturur
Copy link
Member Author

asturur commented May 10, 2023

ok export type works in vscode but for some reason not with the lint command

Copy link
Contributor

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

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

I see there are more actionable code paths that lint didn't fix.
This is a good tool.
Since we are going in this direction how about https://eslint.org/docs/latest/rules/sort-imports?
Before, I wanted it but I thought it was part of the scope of prettier, but of course it isn't. It is the scope of lint. Do you agree?

@@ -23,6 +22,7 @@ import {
TToCanvasElementOptions,
TValidToObjectMethod,
} from '../typedefs';
import { ImageFormat } from '../typedefs';
Copy link
Contributor

Choose a reason for hiding this comment

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

did lint fix this? or did you?
this is a great tool regardless

Copy link
Member Author

Choose a reason for hiding this comment

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

there was a merge conflict and then LINT got angry at my merge resolution. so i run it again

Comment on lines +22 to +23
'@typescript-eslint/consistent-type-exports': 'error',
'@typescript-eslint/consistent-type-imports': 'error',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'@typescript-eslint/consistent-type-exports': 'error',
'@typescript-eslint/consistent-type-imports': 'error',
'@typescript-eslint/consistent-type-exports': [
'error',
{
fixMixedExportsWithInlineTypeSpecifier: true,
},
],
'@typescript-eslint/consistent-type-imports': [
'error',
{
fixStyle: 'inline-type-imports',
},
],

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

image

Copy link
Member Author

Choose a reason for hiding this comment

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

aren't those catched with default values?

Copy link
Member Author

Choose a reason for hiding this comment

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

import { XY, Point } from '../Point';
import { TClassProperties } from '../typedefs';
import type { XY } from '../Point';
import { Point } from '../Point';
import type { TClassProperties } from '../typedefs';

but this similar case was handled.

Copy link
Member Author

Choose a reason for hiding this comment

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

even applying your config i don't get the extra changes.

@asturur
Copy link
Member Author

asturur commented May 11, 2023

i don't want to sort import.
We don't sort vars, we don't sort methods in a class nor property in objects, i don't see why i have to sort imports.

@asturur
Copy link
Member Author

asturur commented May 11, 2023

no i don't like the inlined, there are some situations in which that doesn't work great.
For example at work for me it doens't work.
Is either a recent typescript thing or some setting.

i went to check and inline type import has been introduced in typescript 4.5
i would skip it for now, not sure all tools support it yet

@@ -57,7 +57,7 @@
"test:coverage": "nyc --silent qunit test/node_test_setup.js test/lib test/unit",
"test:visual:coverage": "nyc --silent --no-clean qunit test/node_test_setup.js test/lib test/visual",
"coverage:report": "nyc report --reporter=lcov --reporter=text",
"lint": "eslint --config .eslintrc.js src/**/*.ts",
"lint": "eslint --config .eslintrc.js src",
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 was my issue.
i was getting only files inside the first folders.
VSCode indeed was looking and eslint.rc and workin correctly.

@asturur
Copy link
Member Author

asturur commented May 11, 2023

I updated lint and tsplugins for lint to latest since i was trying to figure out why i didn't see the things

@asturur
Copy link
Member Author

asturur commented May 11, 2023

@ShaMan123 regarding inline vs non inline.
For me separated import exports are very clear to see and follow. the type notation get lost for me in the import / export string if is mixed with the rest.
I also like the idea to stick with default configuration when possible.

@ShaMan123
Copy link
Contributor

So that is why lint ran on my machine and produced a big diff? Because of the merge conflict and the versuons of the tool?

I don't mind about the syntax. Wanted to highlight the options so we make an informed decision.

Copy link
Contributor

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

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

looks valid to me
I didn't check every file though

@asturur
Copy link
Member Author

asturur commented May 11, 2023

do you apply lint with vscode? or with npm run lint? that could have been the difference.
The lint command was wrong.

@asturur asturur merged commit 6175f97 into master May 11, 2023
18 checks passed
@asturur asturur deleted the add-rule-import-type branch May 11, 2023 15:06
tushuhei pushed a commit to tushuhei/fabric.js that referenced this pull request May 13, 2023
@asturur asturur mentioned this pull request May 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants