Use rquickjs-serde implementation #1100
Conversation
|
Looks like the stringify/value-array-abrupt.js tests are failing. There's probably other test262 tests that are failing but it stops executing the test262 suite after the first failure. |
|
For reference, if you checkout all Git submodules in the project and run |
|
Will try it |
|
@jeffcharles All test are passing now, the issues were:
|
|
I'll ask @saulecabrera to review since he wrote the serde pieces. |
| pub fn stringify(val: Value<'_>) -> Result<Vec<u8>> { | ||
| let mut output: Vec<u8> = Vec::new(); | ||
| let mut deserializer = Deserializer::from(val); | ||
| let mut deserializer = Deserializer::from(val).with_strict(); |
There was a problem hiding this comment.
Not to block this PR, but looking at the commentary around with_strict
The implementation tries to make smart guesses when it can, for example the deserializer will fallback to converting
BigInttoString. This is likely not what you want if you implement a serialization that is JSON compliant.This is s why we offer the
strictmode, that will stick to what the behaviour that the Javascript specification defines forJSON. Just switch the method to use it.
Would you consider making the default implementation be the compliant one and make the lossy one opt-in (and potentially unsafe)?. It seems like a potentially footgun otherwise.
saulecabrera
left a comment
saulecabrera
left a comment
There was a problem hiding this comment.
LGTM, thanks for putting this together. Given Javy's dependency on JSON, before landing this PR would you mind adding us as collaborators on the repo and ideally also in crates.io?
|
@saulecabrera @jeffcharles Done, you should have received an invite. |
|
Thanks! |
3 participants
Description of the change
Use the https://github.com/rquickjs/rquickjs-serde
Why am I making this change?
This is just a proposal to see if the test passes. If so it would be nice to centralize around a shared implementation.
Happy to give you guys access to the repository to publish new versions.