From 8b85c963ddf87c485d52cbaedaa8d1830aa9a7ba Mon Sep 17 00:00:00 2001
From: Jimmy Bourassa <jimmy.bourassa@shopify.com>
Date: Fri, 3 May 2024 17:06:12 -0400
Subject: [PATCH] Handle non-serializable values in Array & Object

---
 crates/javy/CHANGELOG.md    |  4 +-
 crates/javy/src/serde/de.rs | 90 +++++++++++++++++++++++++++++++++----
 2 files changed, 85 insertions(+), 9 deletions(-)

diff --git a/crates/javy/CHANGELOG.md b/crates/javy/CHANGELOG.md
index 79e711b0..59715365 100644
--- a/crates/javy/CHANGELOG.md
+++ b/crates/javy/CHANGELOG.md
@@ -10,7 +10,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Changed
 
 - Introduce `rquickjs` to interface with QuickJS instead of `quickjs-wasm-rs`;
-  this version no longer includes re-exports from `quickjs-wasm-rs`. 
+  this version no longer includes re-exports from `quickjs-wasm-rs`.
+- `javy::serde::de::Deserializer` should now match `JSON.stringify`: non-JSON
+  primitives are skipped in Objects and nullified in Arrays.
 
 ## [2.2.0] - 2024-01-31
 
diff --git a/crates/javy/src/serde/de.rs b/crates/javy/src/serde/de.rs
index 6b638e3b..11be8ecd 100644
--- a/crates/javy/src/serde/de.rs
+++ b/crates/javy/src/serde/de.rs
@@ -3,6 +3,7 @@ use crate::serde::err::{Error, Result};
 use crate::serde::{MAX_SAFE_INTEGER, MIN_SAFE_INTEGER};
 use crate::{from_js_error, to_string_lossy};
 use anyhow::anyhow;
+use rquickjs::Null;
 use serde::de::{self, Error as SerError};
 use serde::forward_to_deserialize_any;
 
@@ -200,14 +201,23 @@ impl<'a, 'de> de::MapAccess<'de> for MapAccess<'a, 'de> {
     where
         K: de::DeserializeSeed<'de>,
     {
-        if let Some(kv) = self.properties.next() {
-            let (k, v) = kv.map_err(|e| from_js_error(self.de.value.ctx().clone(), e))?;
-            self.de.value = k.clone();
-            self.de.map_key = true;
-            self.de.current_kv = Some((k, v));
-            seed.deserialize(&mut *self.de).map(Some)
-        } else {
-            Ok(None)
+        loop {
+            if let Some(kv) = self.properties.next() {
+                let (k, v) = kv.map_err(|e| from_js_error(self.de.value.ctx().clone(), e))?;
+
+                // Entries with non-JSONable values are skipped to respect JSON.stringify's spec
+                if !is_jsonable(&v) {
+                    continue;
+                }
+
+                self.de.value = k.clone();
+                self.de.map_key = true;
+                self.de.current_kv = Some((k, v));
+
+                return seed.deserialize(&mut *self.de).map(Some);
+            } else {
+                return Ok(None);
+            }
         }
     }
 
@@ -238,6 +248,13 @@ impl<'a, 'de> de::SeqAccess<'de> for SeqAccess<'a, 'de> {
             self.de.value = self
                 .seq
                 .get(self.index)
+                .map(|v| {
+                    if is_jsonable(&v) {
+                        v
+                    } else {
+                        Null.into_value(self.seq.ctx().clone())
+                    }
+                })
                 .map_err(|e| from_js_error(self.seq.ctx().clone(), e))?;
             self.index += 1;
             seed.deserialize(&mut *self.de).map(Some)
@@ -247,6 +264,17 @@ impl<'a, 'de> de::SeqAccess<'de> for SeqAccess<'a, 'de> {
     }
 }
 
+fn is_jsonable(value: &Value<'_>) -> bool {
+    !matches!(
+        value.type_of(),
+        rquickjs::Type::Undefined
+            | rquickjs::Type::Symbol
+            | rquickjs::Type::Function
+            | rquickjs::Type::Uninitialized
+            | rquickjs::Type::Constructor
+    )
+}
+
 #[cfg(test)]
 mod tests {
     use std::collections::BTreeMap;
@@ -425,4 +453,50 @@ mod tests {
             assert_eq!(vec![1, 2, 3], val);
         });
     }
+
+    #[test]
+    fn test_non_json_object_values_are_dropped() {
+        let rt = Runtime::default();
+        rt.context().with(|cx| {
+            cx.eval::<Value<'_>, _>(
+                r#"
+                var unitialized;
+                var a = {
+                    a: undefined,
+                    b: function() {},
+                    c: Symbol(),
+                    d: () => {},
+                    e: unitialized,
+                };"#,
+            )
+            .unwrap();
+            let v = cx.globals().get("a").unwrap();
+
+            let val = deserialize_value::<BTreeMap<String, ()>>(v);
+            assert_eq!(BTreeMap::new(), val);
+        });
+    }
+
+    #[test]
+    fn test_non_json_array_values_are_null() {
+        let rt = Runtime::default();
+        rt.context().with(|cx| {
+            cx.eval::<Value<'_>, _>(
+                r#"
+                var unitialized;
+                var a = [
+                    undefined,
+                    function() {},
+                    Symbol(),
+                    () => {},
+                    unitialized,
+                ];"#,
+            )
+            .unwrap();
+            let v = cx.globals().get("a").unwrap();
+
+            let val = deserialize_value::<Vec<Option<()>>>(v);
+            assert_eq!(vec![None; 5], val);
+        });
+    }
 }