refactor: strict typing and deprecation fix in type definitions #1573
Conversation
|
I'll review this in more detail tomorrow when I'm at my laptop, but I don't see anything here that I'd call strictly better - can you describe what problem you're seeing that you're trying to solve? |
| @@ -113,33 +113,37 @@ const invalidTestCases: Array<TSESLint.InvalidTestCase<MessageIds, Options>> = [ | |||
| ], | |||
| }, | |||
| // toThrow matchers call the expected value (which is expected to be a function) | |||
| ...toThrowMatchers.map(matcher => ({ | |||
| code: dedent` | |||
| ...toThrowMatchers.map<TSESLint.InvalidTestCase<MessageIds, Options>>( | |||
There was a problem hiding this comment.
We purposely don't define this as it's a complicated type so it's preferred to let typescript infer it
There was a problem hiding this comment.
Got it. I've addressed it in commit b030cc7.
| // toThrow matchers call the expected value (which is expected to be a function) | ||
| ...toThrowMatchers.map(matcher => ({ | ||
| code: dedent` | ||
| ...toThrowMatchers.map<TSESLint.InvalidTestCase<MessageIds, Options>>( |
There was a problem hiding this comment.
Got it. I've addressed it in commit b030cc7.
| @@ -39,9 +39,11 @@ const shouldBeInHook = ( | |||
| case AST_NODE_TYPES.ExpressionStatement: | |||
| return shouldBeInHook(node.expression, context, allowedFunctionCalls); | |||
| case AST_NODE_TYPES.CallExpression: | |||
| const nodeName = getNodeName(node); | |||
There was a problem hiding this comment.
This is a runtime de-optimization - previously we'd only determine the node name if the other condition hasn't matched; now we'll always be getting the node name even if we don't need it
src/rules/valid-expect-in-promise.ts
Outdated
| @@ -110,7 +110,8 @@ const isPromiseMethodThatUsesValue = ( | |||
| } | |||
|
|
|||
| if ( | |||
| ['Promise.resolve', 'Promise.reject'].includes(nodeName as string) && | |||
| nodeName && | |||
There was a problem hiding this comment.
This check is arguably more expensive at runtime - it's very minor, but I'd rather keep the cast
There was a problem hiding this comment.
Got it. I realized I missed considering performance aspects 🙇♂️
I've addressed it in commit b030cc7.
| type: 'array', | ||
| items: { type: 'string' }, | ||
| minItems: 1, | ||
| maxItems: 2, | ||
| additionalItems: false, | ||
| } as const; | ||
| } as const satisfies JSONSchema.JSONSchema4; |
There was a problem hiding this comment.
This is not strictly better than setting the type - there's no need to change this
Here are the two points:
I submitted the proposed changes with the hope of contributing in any way possible. There might be some areas where I lack understanding, so I would greatly appreciate any feedback or guidance you could provide 🙇♂️ |
|
But you're not removing any unneeded type assertions? I appreciate your enthusiasm but I'm not seeing anything here that I'd say is a strong improvement and worth the git churn. The deprecated type is deliberate for now as we're in the middle of supporting both legacy and flat config, and different versions of Since we currently don't publish types, none of this should impact downstream consumers either |
|
Thank you for your explanation. Indeed, as you mentioned, this PR was a minor adjustment. I will close this PR now. You have my apologies for any inconvenience caused 🙇♂️ |
|
No worries - as I said I appreciate the enthusiasm; feel free to have a look over some of our issues and have a go at tackling one of them if you want to help out :) |
|
@y-hsgw just catching up on my general todo list of which re-reviewing this was one as I think there are a couple of interesting possible changes highlighted (#1577 being one). It does look like some of our |
@G-Rath Thanks! I've created a PR at #1578! Please review it. |
I apologize for the sudden PR, but I'd like to propose some strict typing enhancements.
Please consider the following changes:
Note: As this is the first PR for this repository, please feel free to provide any feedback or suggestions if there are any deficiencies.