-
Notifications
You must be signed in to change notification settings - Fork 28
allow filtering #5
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
Conversation
|
Hrm, I'm not massively keen on adding a property onto the objects to pass the predicate around, or the amount of repetition this introduces. What's your exact use-case, perhaps there's another way to do this? |
|
The repetition is to keep the common path fast, it could be DRYer. the use case is as shown in the tests, I have certain keys that can be at arbitrary depths in an object that I would like to exclude from serialization. |
|
How often are you serialising? Could you just do the exclusion separately before passing the value to toJSON? This is how I do it in our app, but we exclude top-level keys, rather than deep ones. I think i'd rather have a closure to pass predicate down than to add properties, interface could be something like: filtered = transit.withPredicate(filterFn);
filter.toJSON(someObj); |
|
did you mean |
|
Yeah, we'd change the reader/writer creation to use a factory function that takes an optional predicate - existing API would be a default invocation of this, and then users would be free to add their own. |
|
what would the new api look like ? |
|
What you've got there is the right idea for the factory. Rather than using a prototype, i'd just have |
|
Updated and squashed |
index.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of exporting createWriter, could you export something like:
function withFilter(predicate) {
var filteredWriter = createWriter(predicate);
return {
toJSON: function(data) {
return filteredWriter.write(data);
},
fromJSON: fromJSON
}
}|
Added |
|
Is everything in order? |
|
Yes, this looks good. One final change though! Could you separate the tests into separate tests per data type? If you don't have time let me know and I'll merge then make the change myself. Thanks for all the work on this. |
|
I am currently swamped. Thanks for your patience. |
|
Released as |
|
Woot ... awesome! |
add a predicate similar to
replacerargument inJSON.stringifyto filter results of the serialization