Skip to content
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

feat: custom serialization #7223

Open
wants to merge 7 commits into
base: build/v2
Choose a base branch
from
Open

feat: custom serialization #7223

wants to merge 7 commits into from

Conversation

wmertens
Copy link
Member

@wmertens wmertens commented Jan 2, 2025

This PR adds symbols to mark objects as (no/yes)serializable via symbols instead of via a Set, adds a symbol prop that will get called on serialization for custom serialization, and provides a new Signal type that lazily manages a non-serializable value.

Note that this is basically the same as useComputed$, except that it is invalidated during SSR so that when the value gets read on the client it will always run the compute function.

Also note that useComputed$ now passes the previous value to the compute function, to keep the implementation compact. This seems like it might be useful, should we document this?

Everything works, looking for comments on implementation, naming etc.

Copy link

changeset-bot bot commented Jan 2, 2025

🦋 Changeset detected

Latest commit: 9070c30

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
@qwik.dev/core Major
eslint-plugin-qwik Major
@qwik.dev/react Major
@qwik.dev/router Major
create-qwik Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Varixo
Copy link
Member

Varixo commented Jan 2, 2025

Looks great, but what about deserialization those objects?

@wmertens
Copy link
Member Author

wmertens commented Jan 2, 2025

Looks great, but what about deserialization those objects?

That's what the createSerialized$ does

@thejackshelton
Copy link
Member

Does this mean we might be able to serialize functions from other libraries, like vanilla js?

@wmertens
Copy link
Member Author

wmertens commented Jan 3, 2025

@Varixo @thejackshelton gaah I had forgotten to push the actual commit 😅

@wmertens
Copy link
Member Author

wmertens commented Jan 3, 2025

Does this mean we might be able to serialize functions from other libraries, like vanilla js?

Yes exactly. You need to add a symbol prop that serializes the value and then provide a deserializer that will get called lazily.

Copy link

pkg-pr-new bot commented Jan 3, 2025

Open in Stackblitz

npm i https://pkg.pr.new/QwikDev/qwik/@qwik.dev/core@7223
npm i https://pkg.pr.new/QwikDev/qwik/@qwik.dev/router@7223
npm i https://pkg.pr.new/QwikDev/qwik/eslint-plugin-qwik@7223
npm i https://pkg.pr.new/QwikDev/qwik/create-qwik@7223

commit: 9070c30

Copy link
Contributor

github-actions bot commented Jan 3, 2025

built with Refined Cloudflare Pages Action

⚡ Cloudflare Pages Deployment

Name Status Preview Last Commit
qwik-docs ✅ Ready (View Log) Visit Preview 9070c30

FEAT: `NoSerializeSymbol`: objects that have this defined will not be serialized

FEAT: `SerializerSymbol`: objects that have this defined as a function will get it called with the object as a parameter during serialization. The function should return the data that should be serialized.
Use this to remove cached data, consolidate things etc.
@wmertens wmertens marked this pull request as ready for review February 6, 2025 15:50
@wmertens wmertens requested review from a team as code owners February 6, 2025 15:50
Comment on lines +849 to +873
: obj instanceof ComputedSignal &&
!(obj instanceof SerializerSignal) &&
(obj.$invalid$ || fastSkipSerialize(obj))
? NEEDS_COMPUTATION
: obj.$untrackedValue$;
if (v !== NEEDS_COMPUTATION) {
discoveredValues.push(v);
if (obj instanceof SerializerSignal) {
promises.push(
(obj.$computeQrl$ as any as QRLInternal<SerializerArg<any, any>>)
.resolve()
.then((arg) => {
let data;
if ((arg as any).serialize) {
data = (arg as any).serialize(v);
}
if (data === undefined) {
data = NEEDS_COMPUTATION;
}
serializationResults.set(obj, data);
discoveredValues.push(data);
})
);
} else {
discoveredValues.push(v);
}
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we could organise it somehow? It is very complicated right now.
What about doing just

if (obj instanceof WrappedSignal) {

} else if (obj instanceof ComputedSignal) {

} else if ...

Copy link
Member

@thejackshelton thejackshelton left a comment

Choose a reason for hiding this comment

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

Very thorough PR. Awesome work ⚡

@@ -4386,7 +4447,7 @@ Creates a computed signal which is calculated from the given function. A compute
The function must be synchronous and must not have any side effects.

```typescript
useComputed$: <T>(qrl: import("./use-computed").ComputedFn<T>) => T extends Promise<any> ? never : import("..").ReadonlySignal<T>
useComputed$: <T>(qrl: ComputedFn<T>) => T extends Promise<any> ? never : ReadonlySignal<T>
Copy link
Member

Choose a reason for hiding this comment

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

Any reason on why the useComputed$ type changes here with useSerializer$?

: obj instanceof ComputedSignal && (obj.$invalid$ || fastSkipSerialize(obj))
: obj instanceof ComputedSignal &&
!(obj instanceof SerializerSignal) &&
(obj.$invalid$ || fastSkipSerialize(obj))
Copy link
Member

Choose a reason for hiding this comment

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

The nested ternaries here were a bit difficult to understand.

It looks to me like computed signal now checks if it's not a serializer signal, if so then if the object is invalid, or can skip serialization, then it needs computation. otherwise it grabs the untracked values.

Copy link
Member

Choose a reason for hiding this comment

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

Also what is v? The current value?

*
* This function must not return a promise.
*/
deserialize: (data: S | undefined, previous: T | undefined) => T;
Copy link
Member

Choose a reason for hiding this comment

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

So on first render, the previous param is -> initial data / inital (3rd argument param of useSerializer$)?

*
* If you do not provide it, the object will be serialized as `undefined`.
*/
serialize?: (customObject: T) => S | Promise<S>;
Copy link
Member

Choose a reason for hiding this comment

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

What's an example of when you may need the deserialize function but not the serialize function?

Copy link
Contributor

@thejackshelton-kunaico thejackshelton-kunaico left a comment

Choose a reason for hiding this comment

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

wrong account 😓

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

4 participants