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

Adapt model serialization to work with the new Single Fetch data loading strategy and streaming format #74

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

acusti
Copy link
Contributor

@acusti acusti commented Jan 21, 2025

Remix v2 introduced Single Fetch as the preferred new data loading strategy, which uses turbo-stream as the streaming format to encode and decode data between the server and the client. as documented in the Breaking Changes section of that guide, this means that:

No more auto-serialization: Naked objects returned from loader and action functions are no longer automatically converted into a JSON Response and are serialized as-is over the wire

as a result, model instances’ toJSON() method is no longer invoked for any model data / query results returned from route loaders, so the raw model instance winds up being serialized instead. i found two different ways to work around this:

Updating auto-serialization to be compatible with new streaming format 3e59403

one approach is to add a ownKeys trap to the model proxy object and override it to provide the keys produced by invoking modelInstance.toJSON(). this approach works because turbo-stream’s encode function uses a utility called flatten to kick off its encoding, and flatten calls partsForObj to handle plain objects, abd partsForObj calls Object.keys() to figure out how to encode the plain object, so customizing ownKeys() to use toJSON() to return the appropriate keys ensures that the object gets serialized by turbo-stream in the form defined by the model (including any customizations, e.g. with the default User model, which removes the password field from the result).

this has a couple of consequences: one, obviously, is that Object.keys() and Object.getOwnPropertyNames() will no longer return the actual raw model fields (e.g. no more attributes, no more relations, no ${prop}, and no fields that the user has chosen to redact from the serialized (“public-facing”) version fo the model. i could see this being a helpful feature and i could also see this being a footgun. the second consequence is that relation field keys are also returned, causing the relation model name to exist on the model (though undefined), which is why we need to check in the get() proxy trap if the value of the prop is not undefined on target, rather than just check for the existence of the prop in target.

Explicit serialization by adding a toJSON method to queries 80ed4bf

another approach is to allow chaining toJSON() to the end of a query to explicitly return a sanitized/serialized version of the model, e.g.:

const articles = await Article.with("user").orderBy("createdAt", "desc").toJSON();

using this from a loader ensures that it explicitly returns the serialized version of a model to provide it to a component, which makes it so that the version of the model that is provided during SSR is exactly equivalent to the version of the model that is made available to the component when rendered on the client. otherwise, the data made available to the component during SSR when using single fetch is the actual model instance, which means it is effectively a superset of the serialized version of the model that is made available to the same component when rendered on the client.


this PR implements both approaches, so that returning the raw query result from a loader will work as expected, but you can also choose to explicitly serialize the result by calling toJSON() at the end of the query. i’m happy to remove either and just go with one or the other if that seems better.

also, while debugging this issue, i discovered that model relations will leak fields redacted by the optional toJSON() overrides, so i fixed that by replacing value.serialize()value.toJSON(): c834819

i also migrated the docs site, examples/remix-cms, and templates/remix to use single fetch: 4e94c50

the lack of any explicit JSON serialization calls on the models when using single fetch meant i needed to refactor the superflare BaseModel and QueryBuilder types. i think the new types are largely more accurate than the previous versions, but it was a headache, and i eventually had to just call it good enough: b63ea4f. one result of the new types is that having the relation type be defined as a union of the Relation | Promise<Relation> no longer works, so i updated those types to all just be the relations, e.g. user: User;: f687a23

and lastly, some assorted cleanup: e59a15c (defunct build.d.ts files, adding sites/app env var) and 16a71ad (typo/grammar fixes in comments + docs).

using serialize skips the potential toJSON() override used to e.g. not include User.password in the serialized form of a User when it is serialized as a relation of another model (e.g. Article in examples/remix-cms)
allows for e.g.:

const articles = await Article.with("user").orderBy("createdAt", "desc").toJSON();

from a loader to explicitly return the serialized version of a model to provide it to a component. this makes it so that the version of the model that is provided during SSR is exactly equivalent to the version of the model that is made available to the component when rendered on the client. otherwise, the data made available to the component during SSR when using single fetch is the actual model instance.
single fetch uses turbo-stream to serialize data, meaning that model.toJSON() is no longer automatically invoked as was the case with the remix json() helper.

turbo-stream’s encode function calls flatten, which defines a partsForObj util that gets called for POJOs and uses Object.keys(): https://github.com/jacob-ebey/turbo-stream/blob/main/src/flatten.ts#L50

customizing ownKeys() to use toJSON() to return the appropriate keys ensures that the object gets serialized by turbo-stream in the form defined by the model ( including any customizations, e.g. the default User model, which removes the password field from the result)

however, calling target.toJSON() means that relation field keys are also returned, which causes the relation model name to exist on the model (though undefined). this is why we need to check in the get() proxy trap if the prop is not undefined on target, rather than just check for the existence of the prop in target.
Copy link

pkg-pr-new bot commented Jan 21, 2025

Open in Stackblitz

npm i https://pkg.pr.new/jplhomer/superflare@74
npm i https://pkg.pr.new/jplhomer/superflare/@superflare/remix@74

commit: 16a71ad

• add an IsSingle type argument to QueryBuilder to track if first() has been invoked
• add the toJSON method to QueryBuilder
• update return types of BaseModel query methods to return a QueryBuilder
• update QueryBuilder promise methods (then/catch) to resolve query results based on if IsSingle (i.e. if first() has been invoked) or not
this allows proper type checking of resolved instances of the model when using single fetch
the placeholder GITHUB_TOKEN in .dev.vars ensures that wrangler includes it in its typegen (worker-configuration.d.ts)
@jplhomer
Copy link
Owner

Nice catch. My thoughts are that, as a developer, Superflare should provide a way for you to either explicitly hide sensitive fields from serialization, whether that's in a toJSON method or with some sort of decorator or hidden: [] class field list.

I like the approach of encoding it into the hasKeys trap, but I also agree that it's a footgun. There's gonna be a point where someone wants to do something on the server with a collection of model instances and is burned because some of their fields are missing or broken.

Looks like turbo-stream has a plugins concept where we could inject and handle this ourselves when Superflare is being used in a RR/Remix context, but I doubt that framework allows us to customize turbo-stream as a consumer. I'd love if Superflare was a generic framework that worked even outside of RR/Remix, and that makes me think we shouldn't bake the solution to a turbo-stream/RR issue directly into our source, but rather find a solution for it at the RR level.

I'll keep thinking on it!

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.

2 participants