Skip to content
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 sloppy filter(Boolean) types #4830

Merged
merged 6 commits into from
Jul 25, 2024
Merged

Fix sloppy filter(Boolean) types #4830

merged 6 commits into from
Jul 25, 2024

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Jul 25, 2024

We were using this as Foo[] pattern which is not good. I got bit by this in my ongoing feed slice refactor because the types were lying. I'm using this approach to get them typed more strongly. It requires extracting variables instead of early returns and some manual annotations, but these actually do get checked.

This surfaced some violations. They're not actually breaking the logic. These two are trivial:

This one is more involved:

It uncovered type errors stemming from parentAuthor being defined as ProfileViewBasic even though technically it could be anything (because we're reading author from a union). We also aren't able to use isProfileViewBasic() here because the object from the server does not have a $type. So instead I'm checking the post itself. If it's a PostView, we can read the author from it. I've had to add some extra logic to deal with blocked posts because the component types were lying.

Test Plan

  • Verified editing thread gates still work for each option.
  • Verified "Load more" shows up and works as expected in Explore for both profiles and feeds.
  • Verified "in reply to..." still shows up in feed
    • Placed breakpoints in both cases where we assign parentAuthor and verified we're able to hit both
    • Verified "Reply to you", "Reply to blocked post", "Reply to <...>" all work

Copy link

render bot commented Jul 25, 2024

Copy link
Collaborator

@pfrazee pfrazee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow I'm disappointed in myself for not having thought of a type-assertion closure like that. Muuuuch better

Copy link

Old size New size Diff
6.63 MB 6.63 MB 528 B (0.01%)

@gaearon gaearon merged commit 4291711 into main Jul 25, 2024
6 checks passed
@gaearon gaearon deleted the fix-sloppy-filters branch July 25, 2024 18:53
@Faithfinder
Copy link
Contributor

Upgrade Typescript to 5.5, then you can just do x => !!x without any typing code around it =P

@janaagaard75
Copy link
Contributor

Upgrade Typescript to 5.5, then you can just do x => !!x without any typing code around it =P

(Unsure if you're being sarcastic.)

I think these kind of clever type conversions should be avoided, because it opens up minefield of edge cases that are hard to remember. See this comparison table: https://dorey.github.io/JavaScript-Equality-Table/. This TypeScript ESLint rule enforces using strict boolean expressions (kinda like in C#): https://typescript-eslint.io/rules/strict-boolean-expressions/. I think the added clarity outweighs the added verbosity.

@Faithfinder
Copy link
Contributor

Upgrade Typescript to 5.5, then you can just do x => !!x without any typing code around it =P

(Unsure if you're being sarcastic.)

I think these kind of clever type conversions should be avoided, because it opens up minefield of edge cases that are hard to remember. See this comparison table: dorey.github.io/JavaScript-Equality-Table. This TypeScript ESLint rule enforces using strict boolean expressions (kinda like in C#): typescript-eslint.io/rules/strict-boolean-expressions. I think the added clarity outweighs the added verbosity.

No sarcasm at all. Not sure what that table has to do with anything, never proposed using == for anything.

As for the rule, didn't enjoy using it, personally, and I don't see it enabled in this codebase from a cursory glance. Boolean and !! are just two ways to write the same thing, in the end. Due to that, typescript-eslint maintainers are considering banning usage of Boolean for bypassing that rule, though probably won't really happen.

Here, my comment was just aiming at simplifying people's lives. Why write <T>, when it's possible to skip it? Alternatively you could use ts-reset, which I bet Dan is familiar with, but that's potentially a more controversial change than just upgrading Typescript.

@gaearon
Copy link
Collaborator Author

gaearon commented Jul 25, 2024

I just copied the first thing I found on SO, happy to merge if there’s some better fix.

@Faithfinder
Copy link
Contributor

I just copied the first thing I found on SO, happy to merge if there’s some better fix.

@gaearon Chose whichever option strikes your fancy: #4840, #4841

@janaagaard75
Copy link
Contributor

janaagaard75 commented Jul 26, 2024

Not sure what that table has to do with anything, never proposed using == for anything.

Sorry. I meant to link to the third tab with the truthy & falsy values in JavaScript. It might just be me, but I find it very hard to remember things like an empty array is true while an empty string is false. It's my experience that forcing expressions to be explicit using ESLint's strict-boolean-expressions avoids a surprising large amount of silly bugs, and that it's well worth the extra verbosity that this adds to the code.

My point is that it's worth avoiding code like if (!!someArray) { ... } and if (!!someString) { ... }.

image

estrattonbailey added a commit that referenced this pull request Jul 31, 2024
* origin/main: (30 commits)
  Only show "followed you back" when appropriate (#4849)
  [Web] Retrigger onEndReached if needed when content height changes (#4859)
  Remove unused NoopFeedTuner (#4856)
  useDedupe callback (#4855)
  [Video] Uploads (#4754)
  Make label required in link components (#4844)
  Boolean filter improvement alternative: TS upgrade (#4840)
  Add label to profile card (#4843)
  Improve a11y on noty feed (#4842)
  Add labels in feed card (#4836)
  Add labels to mod details dialog (#4839)
  Add labels to a few missing places (#4838)
  Add labels in list card (#4837)
  Refactor feed slices (#4834)
  `true` (#4833)
  Replace `import hairlineWidth =` with const (#4831)
  [Videos] Video player - PR #1 - basic player (#4731)
  Bump 1.90 (#4832)
  Fix sloppy filter(Boolean) types (#4830)
  Fuggedaboudit (#4829)
  ...
estrattonbailey added a commit that referenced this pull request Jul 31, 2024
* origin/main: (37 commits)
  Only show "followed you back" when appropriate (#4849)
  [Web] Retrigger onEndReached if needed when content height changes (#4859)
  Remove unused NoopFeedTuner (#4856)
  useDedupe callback (#4855)
  [Video] Uploads (#4754)
  Make label required in link components (#4844)
  Boolean filter improvement alternative: TS upgrade (#4840)
  Add label to profile card (#4843)
  Improve a11y on noty feed (#4842)
  Add labels in feed card (#4836)
  Add labels to mod details dialog (#4839)
  Add labels to a few missing places (#4838)
  Add labels in list card (#4837)
  Refactor feed slices (#4834)
  `true` (#4833)
  Replace `import hairlineWidth =` with const (#4831)
  [Videos] Video player - PR #1 - basic player (#4731)
  Bump 1.90 (#4832)
  Fix sloppy filter(Boolean) types (#4830)
  Fuggedaboudit (#4829)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants