blitz-js / superjson

Safely serialize JavaScript expressions to a superset of JSON, which includes Dates, BigInts, and more.
MIT License
3.88k stars 83 forks source link

`instanceof` breaks in zod validation during dev (Hot Reload Related, probably) #287

Closed Katli95 closed 2 months ago

Katli95 commented 3 months ago

Disclaimer: Not necessarily something wrong with superjson, basically just a subject matter problem. I'm mainly looking for guidance in case anyone has any ideas on how to handle such cases.

I'm running superjson in a next app and using zod for validation. I've got code akin to the sample in the readme:

// SuperJSON registration
import CustomClass from "my/project/utils/my/custom/class";

SuperJSON.registerCustom<CustomClass, string>(
    {
        isApplicable: (v): v is CustomClass => CustomClass.isCustomClass(v),
        serialize: (v) => v.toJSON(),
        deserialize: (v) => new CustomClass(v),
    },
    "my/project/utils/my/custom/class"
);
// zod model
import CustomClass from "my/project/utils/my/custom/class";

const customType = z.instanceof(CustomClass)

This all works fine in a prod build, and during dev as long as I don't trigger a hot reload of some of the related components, but if I do, I start running into some pesky "Input not instance of CustomClass" errors. However, when I save my/project/utils/my/custom/class it all starts working again (probably syncs up the class instance used by all the components).

Any thoughts about a clean solution to my problem? (or even a hacky one since it's just dev šŸ˜…) Should I maybe not be running into this at all?

Skn0tt commented 2 months ago

Hmm, this is interesting šŸ¤” I'm assuming that the hot reload triggers a rerun of SuperJSON.registerCustom, but not a reload of the code that does z.instanceOf. So SuperJSON suddenly uses a different instance of your class to recreate objects, than Zod uses to check instance equality.

I don't see a perfect solution to this. That's in the nature of how classes work in Javascript, and what that does to hot reloading, I guess. But maybe we can find some clever workarounds:

Where in your code base is the SuperJSON.registerCustom call? Could you try putting that into my/project/utils/my/custom/class? That way, the SuperJSON registration should be updated everytime your class or related components change.

If that doesn't already help, maybe you could provide a minimal repro, so I can play around with the problem you're seeing locally?

Katli95 commented 2 months ago

Yeah, thank you for the quick reply, good shout, I'll give it a try and get back with a minimal repro if that doesn't help āœŒļø

Katli95 commented 2 months ago

Alright, took a bit of time but I've got the repro going, again it seems to be a JS / Next problem more than superjson so thank you so much for taking a look.

It actually took a bit of work to get this breaking in the sample (e.g. using next's /app seemed to avoid the issue). The somewhat weird infrastructure is loosely resembling the actual project so I'm not certain all solutions to this repro will be perfectly transferable to the project, but getting your ideas on what's causing the issue and some possible solutions will definitely help us in looking for a permanent solution.

Skn0tt commented 2 months ago

Thanks for the repro! I think I found what's going on:

When a hot reload happens, the files that were changed are re-evaluated. When customClass.ts is re-evaluated, a new class called MyCustomClass is created, and supercedes the old version of the class. But SuperJSON still holds a reference to the old version of your class, and will use this old version to reinstantiate your class during deserialization. To fix this, you need to ensure that .registerCustom is re-executed everytime the Module containing your class is reevaluated. The easiest way to do this is to place the .registerCustom call right below your class:

diff --git a/pages/api/test.ts b/pages/api/test.ts
index 76877c1..9be178f 100644
--- a/pages/api/test.ts
+++ b/pages/api/test.ts
@@ -1,5 +1,3 @@
-import "../../stuff/superjson";
-
 import { NextApiRequest, NextApiResponse } from "next";
 import { test } from "../../stuff/test";

diff --git a/pages/index.tsx b/pages/index.tsx
index 20ef6ca..bb855d3 100644
--- a/pages/index.tsx
+++ b/pages/index.tsx
@@ -1,7 +1,6 @@
 import { useState } from "react";
 import { MyCustomClass } from "../stuff/customClass";
 import SuperJSON from "superjson";
-import "../stuff/superjson";

 export default function Home() {
     const [response, setResponse] = useState<{ success: boolean }>();
diff --git a/stuff/customClass.ts b/stuff/customClass.ts
index d11fc73..a331d25 100644
--- a/stuff/customClass.ts
+++ b/stuff/customClass.ts
@@ -1,6 +1,17 @@
+import SuperJSON from "superjson";
+
 export class MyCustomClass {
     private a = 1;
     private b = 2;

     public add = () => this.a + this.b;
 }
+
+SuperJSON.registerCustom<MyCustomClass, string>(
+    {
+        isApplicable: (v): v is MyCustomClass => v instanceof MyCustomClass,
+        serialize: (v) => "not really serialized but whatever",
+        deserialize: (v) => new MyCustomClass(),
+    },
+    "MyCustomClass"
+);
diff --git a/stuff/superjson.ts b/stuff/superjson.ts
deleted file mode 100644
index 447ca9d..0000000
--- a/stuff/superjson.ts
+++ /dev/null
@@ -1,11 +0,0 @@
-import SuperJSON from "superjson";
-import { MyCustomClass } from "./customClass";
-
-SuperJSON.registerCustom<MyCustomClass, string>(
-    {
-        isApplicable: (v): v is MyCustomClass => v instanceof MyCustomClass,
-        serialize: (v) => "not really serialized but whatever",
-        deserialize: (v) => new MyCustomClass(),
-    },
-    "MyCustomClass"
-);

This is also what SuperJSONs transpilation plugins (https://github.com/blitz-js/superjson?tab=readme-ov-file#using-with-nextjs) do. Let me know if that solves your case!

Katli95 commented 2 months ago

Yeah, this was my best idea as well. Thank you so much for all the effort you've put into this. Unfortunately this doesn't solve my issue šŸ˜£ But it's more than apparent that it isn't caused by superjson, but most likely another element of our setup that seems to be causing zod not to get the latest instance propagated to all references to the model.

So I'll close this issue, thanks again for the help and the awesome library!