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 mask() working incorrectly with union() when an alternative object contains extra unknown props #1196

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dimikot
Copy link

@dimikot dimikot commented Nov 29, 2023

Fixes #803

Basically, it's a more proper rework of my previous PR #629 merged several year ago. The fact is that "mask" approach applies to object structs only, so it's more proper even conceptually to put the masking logic into object() constructor rather than having it in run(). By doing so, I also fix a bug where union() did not work properly with mask(): before, instead of removing the unknown props when a union element matches, it mistakenly threw a "Expected the value to satisfy a union" error.

Test plan

npm run test

Before, the added test threw:

CleanShot 2023-11-28 at 18 39 03@2x

After, all the tests (including the new ones) pass.

@dimikot
Copy link
Author

dimikot commented Nov 30, 2023

@ianstormtaylor could you please take a look? This is a pretty straighforward change.

@dimikot
Copy link
Author

dimikot commented Dec 4, 2023

@ianstormtaylor 🙏

@dimikot
Copy link
Author

dimikot commented Dec 12, 2023

@ianstormtaylor 🙏🙏

@arturmuller
Copy link
Collaborator

Hey @dimikot — I have recently spoken to Ian and I will be helping out with the maintenance of Superstruct from now on.

Getting union to work with mask has actually been on the top of my priority list (it's so useful and a real bummer that it currently doesn't work!) so I am psyched that you've already taken a crack at it! ✌️

I will look into your PR closely next week and get back to you.

@arturmuller
Copy link
Collaborator

Ok, looked through your PR: One thing I am not sure about is whether it a problem that the output of mask with opaque object has changed if an array is passed as value.

// Before
mask([0, 1, 2], object()) // Returns { '0': 0, '1': 1, '2': 2 }

// Your PR
mask([0, 1, 2], object()) // Returns [ 0, 1, 2 ]

Admittedly, it doesn't feel like a very common use-case, and I actually think the new behaviour is more correct, but I would hate to have some patch-version-bump breakages for people in my first couple of releases.

@dimikot
Copy link
Author

dimikot commented Jan 14, 2024

I think I broke it in the old PR #629 actually (this is likely where { '0': 0, '1': 1, '2': 2 } behavior was introduced first), so in the current PR, it's returning back to the roots. But I hear you, I'll try to figure out, how to keep the current behavior, even though it's a little unexpected.

@arturmuller
Copy link
Collaborator

Hi @dimikot — please lmk if you would like to finish this PR. 🙏

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.

Mask with unions
2 participants