Migrate superflare and @superflare/remix from remix v2 → react-router v7 - #73
Closed
acusti wants to merge 14 commits into
Closed
Migrate superflare and @superflare/remix from remix v2 → react-router v7#73acusti wants to merge 14 commits into
acusti wants to merge 14 commits into
Conversation
follow-up to 905ba4c
commit: |
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.
the placeholder GITHUB_TOKEN in .dev.vars ensures that wrangler includes it in its typegen (worker-configuration.d.ts)
this required duplicating node-adapter.js and maintaining it within superflare-remix, because it’s no longer included in the published package’s dist/ directory. here’s a PR that exposes the (from|to)NodeRequest utilities that we depend on in the superflareDevProxyVitePlugin: remix-run/react-router#12774 if it gets merged, we can remove superflare-remix/node.adapter.ts and import those utils alongside cloudflareDevProxy note that i also needed to update superflare-remix’s tsconfig.json module and moduleResolution settings to get it to build without error (presumably due to changes between the published remix vs react-router packages
Contributor
Author
|
as a quick update: i noticed that models weren’t being serialized as i expected due to the change to the single fetch (turbo-stream-based) data loading strategy and streaming format, which is built-in to React Router v7, but was actually introduced in Remix v2, so i wound up needing to make a few functionality changes to make superflare work with Single Fetch. and because those changes don’t require RR 7, i made a new PR that only includes the Single Fetch related changes but still based around Remix: #74 i’m gonna close this PR and open a new one against #74 so that the changes required for RR 7 are reviewable and mergeable separately from the changes required for Remix Single Fetch. |
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
this also meant migrating
examples/remix-cmsfrom the v1 file routing style to the v2 flat routing style (18f3d51) and migrating all examples (the docs site, remix-cms, and the remix template) to using remix v2’s “single fetch” data loading strategy (8b82f7f), and removing the deprecated@remix-run/eslint-configpackage fromapps/siteandexamples/remix-cms.the commit that migrates superflare and @superflare/remix is 80b7046. doing so required duplicating @react-router/dev’s
node-adapter.jsand maintaining it within superflare-remix, because it’s no longer included in the published package’s dist/ directory. i also made a PR in react-router that exposes the (from|to)NodeRequest utilities that we depend on in thesuperflareDevProxyVitePluginand would remove the need for duplicating that file in superflare. if it gets merged, we can removesuperflare-remix/node.adapter.tsand import those utils alongsidecloudflareDevProxy. note that i also needed to update superflare-remix’stsconfig.jsonmoduleandmoduleResolutionsettings to get it to build without error (presumably due to changes between the published remix vs react-router packages).