custom asynchronous deserialization #85
Conversation
…deserialization. Especially, it allows to wait for resolution of references inside custom properties.
…deserialization. Especially, it allows to wait for resolution of references inside custom properties.
|
Offhand this looks pretty good when looking over it! Let me look over this locally. Ideally, every serializr type would return a promise, such that everything would be async, but unfortunately that is not the case right now. I was kind of just thinking that it might be good to combine this with custom, instead of having a |
|
Initially, I was having the thought of combining it as well. To do this you would probably require the propSchema Function to know the parameter types of the serializer / deserializer function. I wasn't aware of a way to do this in VanillaJS. Even in TS this is probably not possible at runtime. Alternatively, you could pass additional options to the propSchema, however this would increase verbosity. Since I thought that explicitly showing somehow the async nature of the schema is important in the class declaration, I thought that giving it a new propSchema would be best. The existing core schemas like object, list etc. don't require async callback imho, or do you have good use case for it that should not be better covered with custom code? |
|
issue #77 we should be also thinking about. internally it was a) which is why I made the PR this way but maybe b) is the more convenient form overall |
|
how about going with option b) and using deserializer.length to detect whether there are 4 parameters in the function definition? This would be the certain indicator that the function expects a callback and serializr could pass the callback to the function instead of running async. Not sure however, whether this works with promisify. |
|
the simple solution with checking for a 4th callback parameter worked fine for me. Ready to pull imo |
|
One last thing, can you bump the version back down to 1.2.0? The version will get bumped when we push a new release to NPM |
|
sure, done |
|
Awesome. I’ll merge and deploy the new version later today. |
| @@ -295,7 +295,7 @@ When deserializing a model elememt / property, the following fields are availabl | |||
|
|
|||
| ## ModelSchema | |||
|
|
|||
| [src/serializr.js:52-52](https://github.com/mobxjs/serializr/blob/8f0e459a1354a7f253a5c1c746269bfa71044093/src/serializr.js#L52-L52 "Source code on GitHub") | |||
| [src/serializr.js:52-52](https://github.com/evoye/serializr/blob/27e5a4ce9cd83f6fa0211f738517b61a2b1a4458/src/serializr.js#L52-L52 "Source code on GitHub") | |||
There was a problem hiding this comment.
The update of README.md is caused by the prepublish script declared in the package.json. All the links have been replaced with which related to you. You should discard the update of this. I will keep the readme being changed accidentally someday.
There was a problem hiding this comment.
since I updated the docs for the custom propschema, the prepublish scripts needs to run to generate the respective readme chapter. Since I cannot do this for the official repo, maybe you can pull, run prepublish and commit the final readme before publishing to npm.
There was a problem hiding this comment.
Yeah ~ I and @alexggordon can do this after your PR merged.
| @@ -71,8 +71,9 @@ export function map(propSchema: PropSchema): PropSchema; | |||
|
|
|||
| export function mapAsArray(propSchema: PropSchema, keyPropertyName: string): PropSchema; | |||
|
|
|||
| export function custom(serializer: (value: any) => any, deserializer: (jsonValue: any) => any): PropSchema; | |||
| export function custom(serializer: (value: any) => any, deserializer: (jsonValue: any, context?: any, oldValue?: any) => any): PropSchema; | |||
| export function custom(serializer: (value: any) => any, deserializer: (jsonValue: any, context: any, oldValue: any, callback: (err: any, result: any) => void) => any): PropSchema; | |||
There was a problem hiding this comment.
An empty line break for readability?
There was a problem hiding this comment.
currently, line breaks are only between different functions, not signature alternatives. We just introduced the latter with the final version.
There was a problem hiding this comment.
👍 Your explanation is reasonable.
I've had the need to wait for the resolution of references during a custom polymorphic deserialization step. The custom propSchema only allowed a synced process as far as I have seen. Therefore, I've added the customAsync propSchema, which allows more control over the process. It should also allow other async deserialization logic, as it is kept as generic and close to the original interface as possible.
I assume, the next version should be 1.2.1, but please check the correctness of the changelog and package.json.
Thanks