-
Notifications
You must be signed in to change notification settings - Fork 142
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
chore: Safe typing for attribute editor #1517
Merged
Merged
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
aff43c8
chore: Safe typing for attribute editor
connorlanigan 59cfc2a
Extract `isSlotFunction`
connorlanigan 8e6d9e0
Merge remote-tracking branch 'origin/main' into unsafe-any/attribute-…
connorlanigan 8027a0b
Exclude public interface type from linting
connorlanigan 8dbbbff
Merge commit '4df7f8d64ab5f5f2e19b44c6b8c15410102d129c' into unsafe-a…
connorlanigan 16558d9
Format file
connorlanigan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
What is happening here? Isn't typescript by itself can figure that this is a function? (from
typeof slot === 'function'
)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.
From the PR description
But I do not see any removed any's in this diff...
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.
From
typeof slot === 'function'
, Typescript infers the type as(item: T, itemIndex: number) => any
. I could not find out precisely why it ends up with that type, but it is caused by the presence ofReact.ReactNode
in line 35.While the
any
type of the returned value of this function is not directly visible in the code, the rule still detects its implicit presence and prevents us from returning an any-typed value. With this new code, the return type of therender
function is nowReact.ReactNode
.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.
In this case I would rather cast the return value than the function:
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.
If we assert the type of the return value, we have to keep that type assertion in sync with the definition of the return type of
FieldRenderable
, which sounds more risky to me than keeping onlyFieldRenderable
in sync only across line 35 and line 38There 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.
This is unsafe either way. We are not fixing any errors, just making the linter happy (which is another big discussion if these rules actually useful).
I prefer the shorter option because it is more readable, cannot see any runtime hazards, really.
When looking into this PR I also learned where this strange
any
is coming from.ReactNode
types are incorrect in our version. Related PR DefinitelyTyped/DefinitelyTyped#56210, "Remove {} from ReactFragment" is the change we needThere 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.
Alternatively, we can write a separate typeguard function
This is also safe and readable to my standards