-
-
Notifications
You must be signed in to change notification settings - Fork 877
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
Clone default values to avoid seemingly surprising behavior #136
Clone default values to avoid seemingly surprising behavior #136
Conversation
Nice this looks good, if not a bug it's definitely a major footgun I would like to be without. If not desired as default behavior for |
That's a valid concern. There is no need to clone the object at the validation time though, instead it can simply be inserted in the generated code: The reason it.useDefault was used in the first place is performance - it is faster to insert the reference than actual object, which is acceptable in cases you don't need to modify the data being validated. So I think it should be based on some option, e.g.
|
or probably better to use |
+1 for |
@jonasfj yes. |
That all seems very reasonable. Just have a couple questions: Should we make the cloning behavior the default (occurs when the option is set to Also, would you like it more if
in the actual template? It's seems like a rather long line otherwise, but I'm not sure what the best taste for this sort of file is. |
Given that it's all optimised for speed a bit surprising is ok. I think it's better if the current behaviour is the default. We will add to options and to filtering data section - it'll be clear enough. Wrapping isn't that important, whatever you prefer, this is clearer though probably: {{=$passData}} = {{? it.opts.useDefaults == 'clone' }}
{{= JSON.stringify($sch.default) }}
{{??}}
{{= it.useDefault($sch.default) }}
{{?}} |
On the other hand you are probably right and less surprising is better. It'll save somebody hours of debugging. In this case let's drop the option idea - nobody will use 'fast' option. And the whole useDefault business will be unused and can be removed. |
On the other hand:), if somebody used some dirty hack where you can modify defaults without recompiling the schema, because it's the same object, the hack would stop working. So let's stick with 'clone' option plan :) |
I think almost anyone running into this is will call it a bug :) Maintaining this obscure "feature" in the name of backwards compatibility, is a bit extreme.. Similar to how Microsoft maintains bugs in Excel because people rely on them :) We could do the 'clone' option, and then make it default in the next major release. But I doubt anyone is intentionally relying on this :) |
Always liked that Microsoft's approach ;) It only came to my mind only because of #103, which is a similar scenario - dynamically update schema dependency. If, for example, you want to have the current timestamp in the default object (or anything else dynamically changing, e.g. fill in IP if the referrer of the request is not supplied), the fact that the default is by reference allows to do it efficiently without recompiling the schema, which would be slow. Filling such dynamic defaults after validation is probably cleaner, I have suggested similar things many times, but in reality people like putting this sort of things in schemas because it allows to change the data structure without having to maintain property references in other places. I agree that this "feature" is close enough to a "bug" :). I'll think whether to remove it completely or to switch the default by the next major version. |
Awesome. I've updated this with the |
Clone default values to avoid seemingly surprising behavior
Great, thank you very much for spotting and fixing. |
@imbstack From v4.0 using object/array literal ("deep-clone") will be the default behaviour and |
Currently if the default value inserted with useDefaults is an object and is subsequently modified on the validated data, the underlying default in the schema is actually modified. That is a confusing sentence, so it's probably just easier to see it in action. The test case in this PR shows an example of how this would occur. Without the change in this PR, it would fail on line 429 of options.spec.js as follows
I'm not 100% sure this is actually a bug, however it does seem to be surprising behavior. If this is not something we want to fix, perhaps the right thing to do is to just add something to the docs that makes mention of this.
Thanks!