fix: revert improve type safety in retargeting proxy setter #930
Conversation
This reverts commit 6d8e742.
❌ Deploy Preview for tresjs-docs failed. Why did it fail? →
|
commit: |
| const setter = setters[prop] | ||
| if (setter && typeof setter === 'function') { | ||
| setter(val, _target, proxy, setTarget) | ||
| if (setters[prop]) { |
There was a problem hiding this comment.
(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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@andretchen0 should merge this one?
There was a problem hiding this comment.
For me, it's ready to merge.
alvarosabu
deleted the
fix/revert-6d8e742646618bbf40b192bfe5dafa376f8780fe
branch
This reverts commit 6d8e742.