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

Evaluate lint rules consistent-type-imports and consistent-type-exports #1283

Closed
samreid opened this issue Jul 16, 2022 · 2 comments
Closed

Comments

@samreid
Copy link
Member

samreid commented Jul 16, 2022

From #1277 the comments for the lint rules consistent-type-imports and consistent-type-exports say:

// TODO: Discuss as a team and probably enable. Can we get WebStorm to automatically import type? If not, it may be too much hassle. Also check if it gives a performance boost. This purportedly enables some tsc optimizations. But is it a hassle if WebStorm or other IDEs don't follow this pattern by default?

Regarding performance the imports rule says:

Type-only imports allow you to specify that an import can only be used in a type location, allowing certain optimizations within compilers.

and the exports rule says:

Type-only imports allow you to specify that an import can only be used in a type location, allowing certain optimizations within compilers.

So the two parts to investigate for this issue are:

  • Can WebStorm automatically import/export using import type and export type?
  • How significant is the performance boost?

If there is a significant performance boost, but webstorm cannot import/export that style, we may try to get the best of both worlds by leveraging the rule's autofix (either intermittently, like once a month/quarter/week, on save, on commit, etc).

@samreid
Copy link
Member Author

samreid commented Jul 16, 2022

I ran a series of type checks under different kinds of changes. This data section is tab-delimited and can be pasted to sublime => excel. Each change type is run 3x for averaging.

tsc all	master	autofix both 
Run initial type check and throw away result		
Run type check with no changes	3463	4024
Run type check with no changes	4116	4215
Run type check with no changes	3974	4576
Add a blank line before `export default class Circuit {`	4437	4530
Add a blank line before `export default class Circuit {`	4757	4970
Add a blank line before `export default class Circuit {`	5811	4335
Add test1?:number to Circuit constructor	7307	7862
Add test2?:number to Circuit constructor	6424	6113
Add test3?:number to Circuit constructor	5798	6223
Add opt1?:number to GOModel and defaults	4884	5082
Add opt2?:number to GOModel and defaults	4827	5054
Add opt3?:number to GOModel and defaults	4838	4975

After running the 1st column, I autofixed both lint rules. Running that autofix took 7 minutes and modified 1378 files like so:

Index: js/lab/model/LabModel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/lab/model/LabModel.ts b/js/lab/model/LabModel.ts
--- a/js/lab/model/LabModel.ts	(revision f622373f19ab3d3065a2f0dc11ad770bd18cf939)
+++ b/js/lab/model/LabModel.ts	(date 1657966242078)
@@ -8,10 +8,11 @@
  */
 
 import optionize from '../../../../phet-core/js/optionize.js';
-import EmptyObjectType from '../../../../phet-core/js/types/EmptyObjectType.js';
+import type EmptyObjectType from '../../../../phet-core/js/types/EmptyObjectType.js';
 import Tandem from '../../../../tandem/js/Tandem.js';
 import centerAndVariability from '../../centerAndVariability.js';
-import CAVModel, { CAVModelOptions } from '../../common/model/CAVModel.js';
+import type { CAVModelOptions } from '../../common/model/CAVModel.js';
+import CAVModel from '../../common/model/CAVModel.js';
 import CAVObjectType from '../../common/model/CAVObjectType.js';
 import CAVConstants from '../../common/CAVConstants.js';
 

In these tests, master was better for all but 2 of the samples:

image

So we do not see a performance boost from this experiment. Therefore, I recommend we disable the rules and do not investigate whether WebStorm streamlines type imports and exports during development.

@zepumph can you please review this issue and see if there are other angles to consider?

@zepumph
Copy link
Member

zepumph commented Aug 11, 2022

All looks correct and excellent. I also wanted to say that I have not noticed any readability concerns with in-lining type imports or exports. I also like the documentation in eslintrc. Ready to close!

@zepumph zepumph closed this as completed Aug 11, 2022
zepumph pushed a commit to phetsims/perennial that referenced this issue Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants