make mask() recursive, allow type() pass unknown properties through in mask=true mode #629
Conversation
dko-slapdash
commented
Jan 26, 2021
•
dko-slapdash
commented
- Make mask() and Struct.mask() running masking recursively, fixes mask() is not recursive #618
- Let type() still keep unknown props when mask() mode is used, fixes let type() still keep unknown props when mask() is used #619
- Add unit tests
- Add a couple more sentences to the docs
- Do not remove masked() coercion for backward compatibility reasons (it may still be useful)
…n mask=true mode - Make mask() and Struct.mask() running masking recursively, fixes ianstormtaylor#618 - Let type() still keep unknown props when mask() mode is used, fixes ianstormtaylor#619 - Add unit tests - Adds a couple more sentence to the docs - Do not remove masked() coercion for backward compatibility reasons (it may still be useful)
|
Hope this PR's code quality passes the library's standards bar; if not - please advice what to improve, I'm happy to do it. |
There was a problem hiding this comment.
Looks great, thank you for the clean PR @dko-slapdash. Just one comment inline about a tweak I think we need to make.
| const ctx: Context = { path, branch } | ||
|
|
||
| if (coerce) { | ||
| if ( |
There was a problem hiding this comment.
I believe the masking should happen after the coercing below, because the coercing is what guarantees we've cloned the input. Otherwise as written this will be mutating the input unexpectedly.
There was a problem hiding this comment.
Cool, will do. In the original masked() implementation, masked's coercion was running first though, and then the struct's own coercion:
superstruct/src/structs/coercions.ts
Line 25 in 140461d
superstruct/src/structs/coercions.ts
Line 80 in 140461d
…ginag values + unit test
|
I just pushed the commit which flips the order of masking and coercion as you requested. Hope you're fine with the difference in behavior in comparison to the upstream where masking was running vice versa, i.e. before the own coercion. I actually think that masking-after-coercion (which you proposed) is better, because it gives the custom coercion functions more freedom (e.g. they can do some backward compatibility work with the previous props before those props are wiped). |
|
@dko-slapdash interesting, yeah I think coercing before masking makes the most sense. I think I want to get rid of |
ianstormtaylor
added
improvement
✶ breaking
|
Thanks Ian! This PR would finalize our migration process from yup. |
|
@dko-slapdash that's great to hear! |
* main: (73 commits) Bump @typescript-eslint/eslint-plugin from 4.14.0 to 4.14.1 Bump @typescript-eslint/parser from 4.14.0 to 4.14.1 Bump rollup from 2.38.0 to 2.38.3 Bump eslint from 7.18.0 to 7.19.0 v0.14.0 update changelog remove masked coercion, in favor of mask make mask() recursive, allow type() pass unknown properties through in mask=true mode (ianstormtaylor#629) Bump eslint-config-prettier from 6.15.0 to 7.2.0 (ianstormtaylor#625) Bump @typescript-eslint/parser from 4.13.0 to 4.14.0 Bump @typescript-eslint/eslint-plugin from 4.13.0 to 4.14.0 Bump rollup from 2.36.2 to 2.38.0 Bump @types/lodash from 4.14.167 to 4.14.168 Bump @types/node from 14.14.21 to 14.14.22 Bump @typescript-eslint/parser from 4.12.0 to 4.13.0 Bump @typescript-eslint/eslint-plugin from 4.12.0 to 4.13.0 Bump @types/node from 14.14.20 to 14.14.21 Bump rollup from 2.36.1 to 2.36.2 Bump eslint from 7.17.0 to 7.18.0 v0.13.3 ...
