chore: bump @typescript-eslint/no-unused-vars to error internally #11173
Conversation
|
github-actions
bot
added
pkg: integration
| @@ -42,7 +42,7 @@ const statusToCodeMap: Record<number, ActionErrorCode> = Object.entries(codeToSt | |||
|
|
|||
| export type ErrorInferenceObject = Record<string, any>; | |||
|
|
|||
| export class ActionError<T extends ErrorInferenceObject = ErrorInferenceObject> extends Error { | |||
| export class ActionError extends Error { | |||
There was a problem hiding this comment.
This T was unused 🔪 . And, as a result, SafeResult's TInput later on becomes unused as well. 🔪
There was a problem hiding this comment.
lgtm, @bholmesdev can you confirm?
There was a problem hiding this comment.
This is required to preserve the error object when using isInputError! It's a strange pattern, I'm aware, but we should definitely keep that around.
There was a problem hiding this comment.
I've documented the reason for this generic type here. The generic is used by ActionInputError (a descendant of ActionError). However, I've found that if you remove this generic from ActionError, the isInputError() utility is unable to apply the generic correctly. If you have a suggestion to avoid this, please let me know @JoshuaKGoldberg. We now have a type test to assert whether removing this generic causes any issue
There was a problem hiding this comment.
Aha! Thanks, that's helpful.
The root problem here is that your isInputError type is not fully type-safe. It violates the "Golden Rule of Generics". Nothing in the body of isInputError actually uses the type parameter T. You could call it like...
const example = new ActionError({ code: "BAD_REQUEST" });
if (isInputError<{ gotcha: "whoops" }>(example) {
example.fields.gotcha;
// ^? string[] | undefined
}...and TypeScript wouldn't know to complain.
Aside: which we recently codified that golden rule in typescript-eslint as
@typescript-eslint/no-unnecessary-type-parameters... but the lint rule doesn't (yet?) have the ability to check type parameters passed as type arguments - see its Limitations). Pity. This wouldn't be caught by linting right now.
I don't have enough context to refactor things to not violate the golden rule. Sometimes the right approach actually is to just eslint-disable-next-line the unused thing and/or only-used-once type parameter. So I'll revert these removals for now with a comment.
There was a problem hiding this comment.
Huh, that's an interesting callout. I've stumbled into TypeScript before learning to rules, so I'm not surprised I violated one 😄
I know what I want to achieve with the generic, I'm just not sure how to go about it differently. I'll make a lower-priority note to revisit that. This is the second time we've had a "what the heck?" moment with that code.
| }, | ||
| }, | ||
| }; | ||
| }, |
There was a problem hiding this comment.
All this 'build:before' hook was doing was assigning to the ssrPluginContext variable... which was then never used. So I believe removing it altogether is safe, right?
JoshuaKGoldberg
changed the title
| @@ -42,7 +42,7 @@ const statusToCodeMap: Record<number, ActionErrorCode> = Object.entries(codeToSt | |||
|
|
|||
| export type ErrorInferenceObject = Record<string, any>; | |||
|
|
|||
| export class ActionError<T extends ErrorInferenceObject = ErrorInferenceObject> extends Error { | |||
| export class ActionError extends Error { | |||
There was a problem hiding this comment.
lgtm, @bholmesdev can you confirm?
| @@ -4,8 +4,7 @@ export const isValidUrl = (s: any) => { | |||
| return false; | |||
| } | |||
| try { | |||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | |||
| const dummy = new URL(s); | |||
| new URL(s); | |||
There was a problem hiding this comment.
Could we even use URL.canParse?
Thought it is better to use the count than to delete it :)
|
We've discussed this among the platform team and think this is a good change. Would need the conflicts resolved first before merging. |
github-actions
bot
added
feat: markdown
There was a problem hiding this comment.
I've responded to the comment above on the unused generic for the ActionError object. I would vote to preserve this generic with a clear comment on why it is present. If we can find a TypeScript refactor for isInputError() to preserves the generic, that would be best.
JoshuaKGoldberg
force-pushed
the
no-unused-vars-error
branch
from
000a673 to
7d587fc
Compare
github-actions
bot
removed
feat: markdown
github-actions
bot
removed
pkg: vue
Changes
Upgrades
@typescript-eslint/no-unused-varsfrom'warn'to'error'in the ESLint config, then resolves the roughly 20 complaints that were hanging around from it. It's a nice amount of deletions, I think. 🙂I don't believe any of the changed types impact users. But just to be safe, added comments inline.