-
Notifications
You must be signed in to change notification settings - Fork 128
Fix TypeScript config #2029
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
Fix TypeScript config #2029
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## trunk #2029 +/- ##
=======================================
Coverage 68.06% 68.06%
=======================================
Files 92 92
Lines 7626 7626
=======================================
Hits 5191 5191
Misses 2435 2435
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There are now 3 files which have issues needing to be addressed:
|
I'm signing off for tonight, so anyone feel free to pick this up. |
plugins/view-transitions/js/types.ts
Outdated
export interface ViewTransitionAnimationConfig { | ||
useGlobalTransitionNames: boolean; | ||
usePostTransitionNames: boolean; | ||
} | ||
|
||
export interface ViewTransitionsConfig { |
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.
Why interface
instead of type
?
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.
FYI I fixed this in 95d0b4d because it's a bug specific to that plugin. For now I used type
for consistency, we can reconsider later / as part of this PR.
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.
I've resolved the conflict by going with type
.
@swissspidy Is there a specific reason you wanted to go with interface
instead?
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.
Typically you'd want to use interface for primitives, and types for things like unions, intersections, and aliases. It's also the general practice in the GB code base as well.
I don't see how interface
would cause any conflicts but don't have a strong opinion here. Use whatever :)
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.
I've seen various takes on that, don't have an opinion really, just not sure what's best. What I recollect reading about this was that it doesn't really matter and "just choose one and stick with it".
Props Gemini 2.5 Pro (preview)
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
With some help from Gemini 2.5 Pro, I've addressed the remaining issues related to Octokit typing. |
When reviewing #2025, I noticed in PhpStorm that some of the types were undefined:
Nevertheless, running
npm run tsc
didn't fail with an error.It turns out this was due to a misconfiguration in in the
tsconfig.json
, due to a mistake by me in 5b97111 as part of #2006.With the fix to the config, these are the errors now uncovered, including the yet-unreleased View Transitions plugin.