-
-
Notifications
You must be signed in to change notification settings - Fork 117
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
Refactor some type related things #1037
Conversation
Run & review this pull request in StackBlitz Codeflow. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice improvements, thank you! I think you will want to update snapshots to ensure tests pass + left one question
@@ -1,8 +1,7 @@ | |||
import type { ClassElement, FunctionParameter } from '../compiler'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this pull Comments and Node too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'm not sure why eslint picked this option - I ran . I agree it looks a bit weird.
I will try to fix it manually.
e22406d
to
f150530
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1037 +/- ##
==========================================
- Coverage 75.95% 75.94% -0.02%
==========================================
Files 89 89
Lines 10849 10844 -5
Branches 1116 1116
==========================================
- Hits 8240 8235 -5
Misses 2602 2602
Partials 7 7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
I usually run |
I'll take a look at the failing e2e test - looks like some angular issue. Let's see what comes up. |
This commit adds an eslint rule `@typescript-eslint/consistent-type-imports` that warns (and automatically fixes) when `import` is used instead of `import type` for importing types. This has minor performance benefits at build time and maybe at runtime, but can also communicate the intent of the import more clearly. For a nice developer experience, it's recommended to enable autofixing fixable eslint issues so that the developer never has to worry about this - it's done completely automatically when the file is saved. Autoformatting with prettier is also recommended. - https://typescript-eslint.io/blog/consistent-type-imports-and-exports-why-and-how/ - https://typescript-eslint.io/rules/consistent-type-imports/
f150530
to
1e49b28
Compare
Ok, it looked like the angular e2e test did not work with this, so I reverted that file and added an eslint ignore for the new rule for it. The e2e tests now also seem to pass locally, let's hope CI agrees 🤞🏻 |
commit: |
refactor: eslint uses
import type
where possibleThis commit adds an eslint rule
@typescript-eslint/consistent-type-imports
that warns (andautomatically fixes) when
import
is used instead ofimport type
forimporting types.
This has minor performance benefits at build time and maybe at runtime,
but can also communicate the intent of the import more clearly.
For a nice developer experience, it's recommended to enable autofixing
fixable eslint issues so that the developer never has to worry about
this - it's done completely automatically when the file is saved.
Autoformatting with prettier is also recommended.
refactor: remove unnecessary type assertion