search

Meteor-Community-Packages / meteor-collection2

A Meteor package that extends Mongo.Collection to provide support for specifying a schema and then validating against that schema when inserting and updating.
https://packosphere.com/aldeed/collection2
MIT License
1.02k stars 108 forks source link

Expose defaultCleanOptions #432

Closed Prinzhorn closed 2 years ago

Prinzhorn commented 3 years ago

Is your feature request related to a problem? Please describe.

I want filter: false or removeEmptyStrings: false globally or per schema.

Describe the solution you'd like

Expose global SimpleSchema.defaultCleanOptions and also add an additional argument to the SimpleSchema constructor to override on a schema level.

Describe alternatives you've considered

Passing filter: false to every single insert, which adds more room for error as it can easily be forgotten. Or imagine a generic function that inserts into multiple collections. How would it know?

Additional context

For my mental model this is something that belongs to the same place that I define the schema. Because that's where I make a conscious decisions what I want and what not. Later when someone calls insert this context is completely lost. To me insert is entirely the wrong place for that. But might be useful for migrations or whatever when you want to force certain options I guess. But in general why would you want the validation to behave differently depending on how the data is inserted? That sounds inconsistent. And consistency is one of the points for using schema in the first place.

Prinzhorn commented 2 years ago

Found it https://github.com/longshotlabs/simpl-schema#set-default-options-for-one-schema

Prinzhorn commented 2 years ago

Actually, let's keep it open for the global version (exposing https://github.com/Meteor-Community-Packages/meteor-collection2/blob/84a97a6169377f736c901360891cb49432666881/package/collection2/collection2.js#L18-L24 somehow). Because I still need to put this in every single constructor.

harryadel commented 2 years ago

Actually, let's keep it open for the global version (exposing somehow). Because I still need to put this in every single constructor.

If the sole purpose of keeping this issue open is for future reference then it's better to close it.

Prinzhorn commented 2 years ago

If the sole purpose of keeping this issue open is for future reference then it's better to close it.

No, as a feature request to expose the defaultCleanOptions options. The original issue where basically two feature requests of which one I just didn't find originally and it already exists.

harryadel commented 2 years ago

Please bear with me as I try to understand this.

No, as a feature request to expose the defaultCleanOptions options

Isn't already exposed as you can assign it along with your schema? Can you please give me a code example of what you want to achieve? Thanks.

Prinzhorn commented 2 years ago

Can you please give me a code example of what you want to achieve?

I want the same sane options for every single schema without having to put it into every single SimpleSchema constructor (can be forgotten or inconsistent -> source of bugs). If I could update defaultCleanOptions then this would work for all schemas automatically.

Replacing const defaultCleanOptions with Collection2.defaultCleanOptions for example would allow exactly that.

https://github.com/Meteor-Community-Packages/meteor-collection2/compare/master...Prinzhorn:patch-1

harryadel commented 2 years ago

Isn't this already achievable via https://github.com/longshotlabs/simpl-schema#set-default-options-for-all-schemas ?

harryadel commented 2 years ago

I've a PR in the works but while I was about to add notes to the README I found about the ability to set this in simpl-schema as referenced in my previous comment. @Prinzhorn

Prinzhorn commented 2 years ago

@harryadel thanks for looking into this. It does not quite solve the same problem. Because what if I'm using the simpl-schema package in the same project for other unrelated purposes. Your draft PR would allow the same options for all the attachSchema use-cases while not messing with the rest.

harryadel commented 2 years ago

I see. If that's the case then I'll take your word for it. Please review https://github.com/Meteor-Community-Packages/meteor-collection2/pull/434 There's something off with the tests that I'm fixing right now. Once it's set and you reviewed it we can merge and release a new version.