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.

Sign up for GitHub

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: revert improve type safety in retargeting proxy setter #930

Merged
merged 3 commits into from
Feb 11, 2025
Merged

fix: revert improve type safety in retargeting proxy setter #930

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cf84887
Revert "fix: improve type safety in retargeting proxy setter"
alvarosabu Feb 10, 2025
a4b34e5
Merge branch 'main' into fix/revert-6d8e742646618bbf40b192bfe5dafa376…
alvarosabu Feb 11, 2025
857050b
refactor(createRetargetingProxy): remove unneeded Partial type from s…
andretchen0 Feb 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions src/utils/primitive/createRetargetingProxy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,8 @@ export function createRetargetingProxy<T extends Record<string | number | symbol
return _target[prop]
},
set(_: any, prop: K, val: T[K]) {
const setter = setters[prop]
if (setter && typeof setter === 'function') {
setter(val, _target, proxy, setTarget)
if (setters[prop]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

(non-blocking)

This was showing up as a build error previously ( #924 ). I haven't been able to track down the issue yet, but I think it's due to TS compiler configuration.

  • The TS configuration appears to be updating from time to time, causing old error-free code to throw new errors.
  • The editor TS configuration sometimes appears to be out of sync with the build TS configuration, causing some errors to be invisible in the editor, but thrown during build/CI.

My guess is that for Partial<Record>, we have a different configuration in the editor vs. build, but I haven't been able to track down exactly where.

But the behavior here is consistent with adding/removing the --exactOptionalPropertyTypes compiler option described in this SO question.

The intention for setters is that all values are functions, never explicitly undefined.

setters is currently a Partial<Record>. Depending on configuration, TS will allow Partial<Record> keys to be set to undefined, even if that's not an allowed value in the specific Record type. That would be consistent with the reported build error.

setters doesn't need to be a Partial. So Partial could be removed from its type in this function's arguments. That will keep TS from complaining that undefined values need to be checked, regardless of --exactOptionalPropertyTypes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I pushed a change that removes the Partial from the setters argument type.

It's been part of createRetargetingProxy since I wrote it, but it was ultimately unnecessary.

Removing it will avoid the build error whether TS is configured with or without --exactOptionalPropertyTypes.

Copy link
Member Author

Choose a reason for hiding this comment

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

The only thing I can think of is that the difference between editor and build could be a difference in the versions available in the node modules perhaps? Lets keep an eye on the build typescript issue to see what could be causing the difference between a version or another

Copy link
Member Author

Choose a reason for hiding this comment

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

@andretchen0 should merge this one?

Copy link
Contributor

@andretchen0 andretchen0 Feb 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

For me, it's ready to merge.

setters[prop](val, _target, proxy, setTarget)
}
else {
_target[prop] = val
Expand Down