-
Notifications
You must be signed in to change notification settings - Fork 8
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: implement TS exactOptionalPropertyType rule #90
refactor: implement TS exactOptionalPropertyType rule #90
Conversation
🦋 Changeset detectedLatest commit: 377f636 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to 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.
Lot of things I like here, but a lot of coercions that I don't think I understand.
As per usual, I only flagged the first instance of any issues I had (and not every repeat)
PS:
- I updated the PR title per Conventional Commits. Even though we use changesets for changelogs, git history is still important for people to navigate (ps: also why we squash PRs)
- I marked the PR as
ready for review
, since my understanding from today's sync is that it is ready for review, it just might possibly not be ready to be merged. Try and usedraft PRs
for things you're still actively working on , and use the PR description to indicate things like merge blockers, PR dependencies, etc.
(Draft or normal PR, you still can assign individuals for review and dismiss stale/re-ping them for another review)
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.
Tl;dr
- For passing props: whenever possible, we should avoid inlining logic inside
<Components ... />
, and instead assign that logic to a variable. - For our types: we should add
| undefined
to every single optional type unless there is an explicit reason to treat?:
different from an explicitundefined
. - For our components: if there's a value that we need to coerce to a different fallback shape to satisfy a type, it's likely a sign that we need to fix the type of either the prop or the variable.
- Always use
propName={ ...( var && { propName: var }
overpropName={ varName || undefined }
, since this rule means explicitly undefined props are not the same as optional ones.
PS: Don't forget to check previously unresolved discussions before requesting re-review.
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.
My previous review remains relevant; no need to ping me again for review until all the unresolved conversations have been addressed.
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.
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 got sick of repeating myself so I handled the open comments in 166fb4f
Only things remaining are:
- @ayushnirwal should we be adding
| undefined
to all of our undefined types in@core/props
or@types
? (IMO yes, since unless we differentiate on behavior, we should absorb the headache for end-users who also want theexactOptionalPropertyType
enforced locally, but I defer to you). - after a decision/change per the above (so the package diff is accurate), we need to
npm run changeset
Most of the types do not need |
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.
@ayushnirwal should we be adding | undefined to all of our undefined types in @core/props or @types? (IMO yes, since unless we differentiate on behavior, we should absorb the headache for end-users who also want the exactOptionalPropertyType enforced locally, but I defer to you).
Most of the types do not need
| undefined
according to my understanding. I see no cases where we need to set a prop (to a block renderer) to be undefined. It can be just left undefined. The same logic is applied to type describing env vars. Other remaining types (related to block manager) may need| undefined
but they are not fully "baked" yet. Decisions for them can be made when we do that.
While I still disagree with the former:
- (the only reason a user needs to care about explicit/implicit
undefined
s is because they are enablingexactOptionalPropertyType
, which is a practice we want to encourage. So we should make it easy for them by ensuring we handle explicit/implicitundefined
s in the components they might use. "Why" they might be passing an explicit undefined to one of our component props shouldn't matter to us or it risks becoming a source of tech debt.)
I very much agree with the latter:
- we can deal with this incrementally as needed, and adding
| undefined
to a type is a nonbreaking change.
What
Implemention of
"exactOptionalPropertyTypes": true
ruleWhy
stricter rules around how ts handles properties on type or interfaces which have a ? prefix.
Related Issue(s):
https://github.com/rtCamp/headless/issues/317
How
Testing Instructions
Screenshots
Additional Info
Without this flag enabled, there are three values which you can set colorThemeOverride to be: “dark”, “light” and undefined .
Checklist
npm run changeset
.