Skip to content

feat(evaluate): serialize map and set#26730

Merged
dgozman merged 3 commits intomicrosoft:mainfrom
sand4rt:serialize-maps-and-sets
Aug 28, 2023
Merged

feat(evaluate): serialize map and set#26730
dgozman merged 3 commits intomicrosoft:mainfrom
sand4rt:serialize-maps-and-sets

Conversation

@sand4rt
Copy link
Contributor

@sand4rt sand4rt commented Aug 27, 2023

closes: #24040

@sand4rt sand4rt changed the title feat(evaluate): serialize map and sets feat(evaluate): serialize map and set Aug 27, 2023
@github-actions

This comment has been minimized.

@sand4rt sand4rt force-pushed the serialize-maps-and-sets branch from 1efd15f to 507a38d Compare August 27, 2023 19:47
@github-actions

This comment has been minimized.

@mxschmitt
Copy link
Contributor

I'd do something like this instead which follows our existing serialisation infra, which would also work with circular objects, complex data types inside Map etc. This would be worth adding some tests.

diff --git a/packages/playwright-core/src/protocol/serializers.ts b/packages/playwright-core/src/protocol/serializers.ts
index 3e1f99a41..813ab22cb 100644
--- a/packages/playwright-core/src/protocol/serializers.ts
+++ b/packages/playwright-core/src/protocol/serializers.ts
@@ -76,9 +76,9 @@ function innerParseSerializedValue(value: SerializedValue, handles: any[] | unde
   if (value.r !== undefined)
     return new RegExp(value.r.p, value.r.f);
   if (value.m !== undefined)
-    return new Map(Object.entries(JSON.parse(value.m)));
+    return new Map(Object.entries(innerParseSerializedValue(value.m, handles, refs)));
   if (value.se !== undefined)
-    return new Set(JSON.parse(value.se));
+    return new Set(innerParseSerializedValue(value.se, handles, refs));
 
   if (value.a !== undefined) {
     const result: any[] = [];
@@ -150,9 +150,9 @@ function innerSerializeValue(value: any, handleSerializer: (value: any) => Handl
     return { s: `${error.name}: ${error.message}\n${error.stack}` };
   }
   if (isMap(value))
-    return { m: JSON.stringify(Object.fromEntries(value)) };
+    return { m: innerSerializeValue(Object.fromEntries(value), handleSerializer, visitorInfo) };
   if (isSet(value))
-    return { se: JSON.stringify(Array.from(value)) };
+    return { se: innerSerializeValue(Array.from(value), handleSerializer, visitorInfo) };
   if (isDate(value))
     return { d: value.toJSON() };
   if (isURL(value))
diff --git a/packages/playwright-core/src/protocol/validator.ts b/packages/playwright-core/src/protocol/validator.ts
index 71f1ffbd0..c05a349a5 100644
--- a/packages/playwright-core/src/protocol/validator.ts
+++ b/packages/playwright-core/src/protocol/validator.ts
@@ -58,8 +58,8 @@ scheme.SerializedValue = tObject({
   d: tOptional(tString),
   u: tOptional(tString),
   bi: tOptional(tString),
-  m: tOptional(tString),
-  se: tOptional(tString),
+  m: tOptional(tType('SerializedValue')),
+  se: tOptional(tType('SerializedValue')),
   r: tOptional(tObject({
     p: tString,
     f: tString,
diff --git a/packages/playwright-core/src/server/isomorphic/utilityScriptSerializers.ts b/packages/playwright-core/src/server/isomorphic/utilityScriptSerializers.ts
index 9be8d04a3..90e4b913e 100644
--- a/packages/playwright-core/src/server/isomorphic/utilityScriptSerializers.ts
+++ b/packages/playwright-core/src/server/isomorphic/utilityScriptSerializers.ts
@@ -20,8 +20,8 @@ export type SerializedValue =
     { d: string } |
     { u: string } |
     { bi: string } |
-    { m: string } |
-    { se: string } |
+    { m: SerializedValue } |
+    { se: SerializedValue } |
     { r: { p: string, f: string} } |
     { a: SerializedValue[], id: number } |
     { o: { k: string, v: SerializedValue }[], id: number } |
@@ -105,9 +105,9 @@ export function source() {
       if ('bi' in value)
         return BigInt(value.bi);
       if ('m' in value)
-        return new Map(Object.entries(JSON.parse(value.m)));
+        return new Map(Object.entries(parseEvaluationResultValue(value.m)));
       if ('se' in value)
-        return new Set(JSON.parse(value.se));
+        return new Set(parseEvaluationResultValue(value.se));
       if ('r' in value)
         return new RegExp(value.r.p, value.r.f);
       if ('a' in value) {
@@ -178,9 +178,9 @@ export function source() {
       return { bi: value.toString() };
 
     if (isMap(value))
-      return { m: JSON.stringify(Object.fromEntries(value)) };
+      return { m: serialize(Object.fromEntries(value), handleSerializer, visitorInfo) };
     if (isSet(value))
-      return { se: JSON.stringify(Array.from(value)) };
+      return { se: serialize(Array.from(value), handleSerializer, visitorInfo) };
 
     if (isError(value)) {
       const error = value;
diff --git a/packages/protocol/src/channels.ts b/packages/protocol/src/channels.ts
index c57877321..9e7abcbd1 100644
--- a/packages/protocol/src/channels.ts
+++ b/packages/protocol/src/channels.ts
@@ -180,8 +180,8 @@ export type SerializedValue = {
   d?: string,
   u?: string,
   bi?: string,
-  m?: string,
-  se?: string,
+  m?: SerializedValue,
+  se?: SerializedValue,
   r?: {
     p: string,
     f: string,
diff --git a/packages/protocol/src/protocol.yml b/packages/protocol/src/protocol.yml
index bf7cd020e..72f4bf809 100644
--- a/packages/protocol/src/protocol.yml
+++ b/packages/protocol/src/protocol.yml
@@ -83,9 +83,9 @@ SerializedValue:
     # String representation of BigInt.
     bi: string?
     # String representation of Map.
-    m: string?
+    m: SerializedValue?
     # String representation of Set.
-    se: string?
+    se: SerializedValue?
     # Regular expression pattern and flags.
     r:
       type: object?

@sand4rt sand4rt force-pushed the serialize-maps-and-sets branch from 507a38d to 9c9b5e0 Compare August 28, 2023 14:38
@github-actions

This comment has been minimized.

@sand4rt
Copy link
Contributor Author

sand4rt commented Aug 28, 2023

I'd do something like this instead which follows our existing serialisation infra, which would also work with circular objects, complex data types inside Map etc. This would be worth adding some tests.

Thanks for the pointers. I've updated the PR.

@sand4rt sand4rt force-pushed the serialize-maps-and-sets branch from 4478abc to 01a3f71 Compare August 28, 2023 17:13
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copy link
Contributor

@dgozman dgozman left a comment

Choose a reason for hiding this comment

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

Looks great, just some nits.

@sand4rt sand4rt requested a review from dgozman August 28, 2023 20:49
@github-actions
Copy link
Contributor

Test results for "tests 1"

5 flaky
⚠️ [chromium] › library/tracing.spec.ts:239:5 › should not include trace resources from the previous chunks
⚠️ [chromium] › page/page-event-request.spec.ts:130:3 › should report navigation requests and responses handled by service worker with routing
⚠️ [playwright-test] › ui-mode-test-progress.spec.ts:167:5 › should update tracing network live
⚠️ [webkit] › library/browsercontext-reuse.spec.ts:50:1 › should reset serviceworker
⚠️ [playwright-test] › ui-mode-test-progress.spec.ts:22:5 › should update trace live

25555 passed, 595 skipped
✔️✔️✔️

Merge workflow run.

@dgozman dgozman merged commit ee203b7 into microsoft:main Aug 28, 2023
dgozman added a commit to dgozman/playwright that referenced this pull request Sep 20, 2023
dgozman added a commit that referenced this pull request Sep 21, 2023
dgozman added a commit to dgozman/playwright that referenced this pull request Sep 21, 2023
dgozman added a commit that referenced this pull request Sep 21, 2023
Germandrummer92 pushed a commit to OctoMind-dev/playwright that referenced this pull request Oct 27, 2023
Germandrummer92 pushed a commit to OctoMind-dev/playwright that referenced this pull request Oct 27, 2023
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.

[Feature] Support passing Map/Set as props in component testing

3 participants