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

Skip serialization / deserialization #32

Closed
wants to merge 3 commits into from
Closed

Skip serialization / deserialization #32

wants to merge 3 commits into from

Conversation

nathanstitt
Copy link
Contributor

This allows custom serializers/deserializers to conditionally skip setting properties.

My use case for this is that I'm migrating a large app that used ampersand-state to Mobx and Serializr.

As part of the migration I've extracted some decorators I found useful to mobx-decorated-models. It's a bunch of decorators that make using Serializr with Mobx a bit easier for my use case. However ampersand-state has the concept of session attributes, which are fields that behave just like other attributes in that they can be optionally set from the server, but are not serialized for saving.

They're pretty useful for allowing the server to send pre-computed values for things that are difficult/impossible to generate client-side. But it makes no sense for the client to save the values back to the server.

This change will allow me to support that type of functionality. I'm happy to rework in a different fashion if you think something else would be a better way to support this.

This allows custom serializers to conditionally skip setting properties.

Can be useful for creating schema's where fields should be settable
from incoming JSON, but the fields should not be saved.  Or maybe
fields should be computed client-side and server-side data should
conditionally be ignored.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 94.09% when pulling e36353a on nathanstitt:skip-when-undefined into f6fd1bb on mobxjs:master.

@nathanstitt
Copy link
Contributor Author

nathanstitt commented Feb 12, 2017

I've also got a bit more code that passes the property name and the parent object to the serializer function. I've found that info's needed to prevent circular references in some situations.

Commit is: https://github.com/nathanstitt/serializr/commit/eb079fbcb460ab1fe8855d3c0401ed30077b2bdb

I'll make some specs and a follow up PR with it once this one clears (assuming it does?)

@mweststrate
Copy link
Member

I am wondering whether this change doesn't skip properties to aggresively. It doesn't seem possible to serialize undefined values now at all? What about only skipping undefineds for custom attributes, or maybe having a special SKIP value to indicate skipping attributes. In that case a custom serializer could return serializer.SKIP to flag that the field should not be serialized?

@mweststrate
Copy link
Member

Passing parent / propname sounds like useful change indeed. Can you elaborate / demonstrate your case a bit? But it sounds like a sane change

@nathanstitt
Copy link
Contributor Author

@mweststrate Thanks for taking a look at the change. The SKIP value idea is also good and would work great for me. That's essentially what I'm doing except using undefined as the SKIP. My reasoning for that is that undefined means essentially "not set" because it's also what's returned for non-existing properties. But you're correct it's probably better to be explicit. I'll rework to do that (probably using a Symbol).

What I'm attempting to do with passing along the propname is to be able to conditionally set or merge values. Currently custom serializers don't have enough information to be able to find the existing value so they can apply smart updates.

For some type of models, if the property has quite a bit of extra information that is calculated, it can be expensive to throw it out and start over, it's better to pass it the new values from serializr and let it update itself.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 93.882% when pulling 306f6f1 on nathanstitt:skip-when-undefined into f6fd1bb on mobxjs:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 93.882% when pulling b19ed9c on nathanstitt:skip-when-undefined into f6fd1bb on mobxjs:master.

@nathanstitt
Copy link
Contributor Author

@mweststrate I've made the changes and added the property name commit as well. Looks like coverall's isn't happy even though I've added a few specs :(

Happy to rework if you've got any other suggestions. Thanks!

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 93.882% when pulling a8d70b5 on nathanstitt:skip-when-undefined into 0e9b7d9 on mobxjs:master.

@nathanstitt nathanstitt changed the title Skip properties when serializing function returns undefined Skip serialization / deserialization Mar 9, 2017
function mrFactory() {
// Indicate properties should be skipped
var SKIP = Symbol('SKIP')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that Symbol is not standard ES5, so something like typeof Symbol !== "undefined" ? Symbol('SKIP') : { SKIP: true } would be better. I'll try to merge later today, so I might process that change myself

@mweststrate
Copy link
Member

Released as part of 1.1.11

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.

None yet

3 participants