Skip to content

provide a generic serializer #4443

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

Merged
merged 5 commits into from
Jun 10, 2020
Merged

provide a generic serializer #4443

merged 5 commits into from
Jun 10, 2020

Conversation

bobzhang
Copy link
Member

@bobzhang bobzhang commented Jun 5, 2020

cc @ryyppy @jfrolich
There is a bug in v8, https://bugs.chromium.org/p/v8/issues/detail?id=10595
so this may not be that useful (it is not uncommon to have deeply nested objects, take list for example)
Since JSON does not have undefined, we encode it a s a magic value, and when de-serializing, we convert it back to undefined.


val deserializeExn : string -> 'a

val serializeExn : 'a -> string option
Copy link
Member

Choose a reason for hiding this comment

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

why does serializeExn return an string option?

Copy link
Member Author

Choose a reason for hiding this comment

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

JSON.stringify(Symbol()) returns undefined, maybe we just raise in this case to simplify things

@ryyppy
Copy link
Member

ryyppy commented Jun 5, 2020

Just to describe this PR for my own understanding, this is how you could e.g. use the serialization functions for NextJS limitations where I have option values for nullability reasons:

type user = {
  name: string,
  age: option(int), 
};

type props = { userStr: string };

let default = (props: props) => {
  // user = `{ name: "Patrick", age: None }`
  let user = Js.Json.deserializeExn(props.userStr);

 // We can now use the age like an option value
 let ageEl = Belt.Option.getWithDefault(React.null, (age) => age->Belt.Int.toString->React.string);
 <div>
  {user.name->React.string}  {ageEl}
 </div>
};

let getStaticProps = (_ctx) => {
  let mockUser = { name: "Patrick", age: None};

  let props = {
    // userStr = `{ "name": "Patrick" }`
    userStr: Js.Json.serializeExn(mockUser)
  };

  {
    "props": props
  }
};

So these functions would essentially be a (better?) integrated part of the platform to serialize any data to JSON compliant structures?

@bobzhang
Copy link
Member Author

bobzhang commented Jun 5, 2020

I would say for prototype or not highly invested project (personal projects).
To serialize data in a high quality versioned style, you need a schema like protobuf.

deserialize is not type safe, since its return type can be any depending on user annotations, what's the better name, unsafeDeserializeExn?

@ryyppy
Copy link
Member

ryyppy commented Jun 5, 2020

hmm, i am not entirely sure if this is the right way yet.

E.g. I don't see a direct benefit of using a magic value to represent the case of a non-existing option value, since it actually does also work to just strip away the value entirely (as I proposed in my latest blog post).

I think this topic needs further proper discussion with detailed edge-cases / a user-land prototype before there should be another function in Js.Json?

@bobzhang
Copy link
Member Author

bobzhang commented Jun 5, 2020 via email

@bloodyowl
Copy link
Collaborator

since it actually does also work to just strip away the value entirely

@ryyppy That works for records, but for arrays we still need a "magic" value to encode None

JSON.stringify({foo: undefined, bar: 1})
// "{\"bar\":1}"
JSON.stringify([undefined, 1, undefined])
// "[null, 1, null]"

Comment on lines 187 to 188
let deserializeExn (s: string) : 'a =
patch (parseExn s)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason why you don't use the second JSON.parse argument like the following?

JSON.parse(s, function(_key, value) {
  if(value && value.RE_PRIVATE_NONE === true) {
    return undefined
  };
  return value
});

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately this does not work, you can try it

Copy link
Collaborator

Choose a reason for hiding this comment

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

Screenshot 2020-06-06 at 14 00 05

Tried on Chrome and Safari and it does seem to work? The only difference I see is that it makes the arrays sparse, but that doesn't seem to be an issue given how BS's runtime interacts with them?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh yeah, that'd break Js.Array.*. Nevermind 😄

Copy link
Collaborator

Choose a reason for hiding this comment

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

Something like this would work, I'm not sure if that'd optimise better in browser VMs

let pairs = [];
let parsed = JSON.parse(s, function(key, value) {
  if(value && value.RE_PRIVATE_NONE === true) {
    pairs.push([this, key]);
    return undefined
  };
  return value
});
pairs.forEach(([obj, key]) => obj[key] = undefined)
parsed;

Copy link
Member Author

Choose a reason for hiding this comment

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

it does not work for me, do you have a working example? is this semantics documented somewhere?

Even if it works, it seems the IC is still changed

Copy link
Member Author

Choose a reason for hiding this comment

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

let p = s => {var parents = [];
var obj = JSON.parse(s, function(key, value) {
  if(value && value.RE_PRIVATE_NONE === true) {
    parents.push({par:this, key});
  };
  return value
}); for(let {par,key} of parents){par[key]=undefined}; return obj }

This looks good to me

Copy link
Collaborator

Choose a reason for hiding this comment

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

Documented here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/parse#Using_the_reviver_parameter

I made a rough benchmark in Chrome and Safari and it seems that your original solution is consistently faster, so the only benefit left would be runtime size. About IC it seems like returning undefined from the reviver function will necessarily call some delete, but I'm wondering if using null as a placeholder until replacing "by hand" with undefined would help.

Copy link
Member Author

Choose a reason for hiding this comment

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

It maybe that JSON.parse without reviver is specialised.
About the bundle size, my implementation is quite small, if it is indeed faster, we can keep it.
It is good to know an alternative solution!

Copy link
Member Author

Choose a reason for hiding this comment

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

I am thinking we can do the same thing with stringify without using callback, it may be faster

@bobzhang
Copy link
Member Author

bobzhang commented Jun 8, 2020 via email

@bobzhang
Copy link
Member Author

bobzhang commented Jun 9, 2020

ping @ryyppy , what's the semantics of next.js toJSON protocol? Is there any docs about it, I expect next.js would require more than serialized to JSON? does it need be deserialized back?

@ryyppy
Copy link
Member

ryyppy commented Jun 9, 2020

@bobzhang NextJS currently has two APIs for hydrating React components via props

The docs are vague on what the shape of props actually is, but it turned out to be plain JSON... but the React component doesn't show that there is any JSON serialization going on.

Here the docs example in JS:

function Blog({ posts }) {
  // Render posts (where `posts` is already an array of posts)....
}

// This function gets called at build time
export async function getStaticProps() {
  // Call an external API endpoint to get posts
  const res = await fetch('https://.../posts')
  const posts = await res.json()

  // By returning { props: posts }, the Blog component
  // will receive `posts` as a prop at build time
  return {
    props: {
      posts,
    },
  }
}

export default Blog

In this example, you don't see any serialization going on, since Next assumes that you are passing plain JS objects without any functions / this context etc. It's doing the transformation for you already in the background, so you can just use the data as is in the component... that's why in development mode, Next will warn you if you are returning data that is not JSON compliant.

Regarding the protocol: Next is literally using JSON.stringify for serialization and JSON.parse for reconstructing the value

@bobzhang
Copy link
Member Author

@ryyppy thanks for the point. What will you happen if you pass an arbitrary json object to next.js.
Take your input for example:
instead of

 {
  diet_kind: undefined
};
{ diet_kind : {RE_PRIVATE_NONE:true}}

This is a valid json object, how will next.js interpret it?

@ryyppy
Copy link
Member

ryyppy commented Jun 10, 2020

This would be serialized correctly by Next but we'd need to type the props as some JS.Json.t object and convert it in the component body to the actual value

@bobzhang
Copy link
Member Author

bobzhang commented Jun 10, 2020 via email

@ryyppy
Copy link
Member

ryyppy commented Jun 10, 2020

in the html output: "{ \"diet_kind\" : {\"RE_PRIVATE_NONE\":true}}"

when mounted in JS: { diet_kind : {RE_PRIVATE_NONE:true}}

@bobzhang
Copy link
Member Author

bobzhang commented Jun 10, 2020 via email

@ryyppy
Copy link
Member

ryyppy commented Jun 10, 2020

Yes, the example above is the desired output. The reason why Next does this is bc it pre renders the react component data as a JSON string in the static html output and as soon as the user loaded the website it will parse the string data and pass it to the component

@bobzhang
Copy link
Member Author

@ryyppy so our solution will work for you. Instead of a serializing/deserization to string, you need a RTT function to plain json object

@bloodyowl
Copy link
Collaborator

@ryyppy Maybe we can open an issue in next.js and ask them if they'd be okay to let users provide a custom (de)serialiser in next.config.js

@ryyppy
Copy link
Member

ryyppy commented Jun 10, 2020

@bobzhang yep
@bloodyowl do you think a config would be useful? i feel like having props typed as json and then doing this one single conversion ourselves is less confusing (at least until we figured out how tedious this actually is)

@bloodyowl
Copy link
Collaborator

I think that in terms of API, it'd be simpler if we were to tell people:

Here are two lines to copy once in your next.config.js

serializer: require("bs-platform/.../json").serializeExn
deserializer: require("bs-platform/.../json").deserializeExn

vs telling them to call the function in both places for each component.

@bobzhang
Copy link
Member Author

updated the interface of val serializeExn : 'a -> string

@ryyppy I planned to add a pair of functions for 'a -> Json.t, but you can do this on user side, using JSON.parse. It seems as long as we preserve s -> serialize -> deserialize = s, it will be useful in your cases.

For the record, the patching on serialisation is not done since you don't want to modify the existing

@bobzhang bobzhang merged commit 2554e2a into master Jun 10, 2020
@bobzhang
Copy link
Member Author

bobzhang commented Jun 10, 2020

depends on user demand, we may add support for Date, bigint in the future

@ryyppy
Copy link
Member

ryyppy commented Jun 10, 2020

thanks @bobzhang .. will upgrade reasonml.org to v8 to use this when the release is out.

@bloodyowl yeah, might be easier for some ppl... just wanted to have a chance to testflight this feature set before asking for new features in Next 😄

@bobzhang
Copy link
Member Author

this is the nodejs convention

const buf = Buffer.from([0x1, 0x2, 0x3, 0x4, 0x5]);
const json = JSON.stringify(buf);

console.log(json);
// Prints: {"type":"Buffer","data":[1,2,3,4,5]}

Maybe we should follow the same convention?

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.

3 participants