-
Notifications
You must be signed in to change notification settings - Fork 4
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
ReScriptify Server and implement asset loading #51
Merged
Conversation
This file contains 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
7585638
to
06afbe7
Compare
zth
reviewed
Jun 28, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great overall, nice work! I have a few questions/thoughts that I'm thinking you can probably address quite quickly. Good work!
06afbe7
to
0ce6211
Compare
This is the start of converting the server.mjs to ReScript which requires the addition of some bindings for server side code. The main goal of these changes is to enable moving the React rendering side and its effects (emission of assets) entirely into the EntryServer. This should make it possible to create a pattern around renderTo*Stream where utilities (e.g. the `PreloadInsertingStream` and others) can be wired up to work regardless of the server environment. Typing the original asset to script conversion code was too difficult to complete in a day, so to ensure this commit brings code into a buildable state that can be shared and interated upon. There are two important TODO's that have significant changes from the working code in another project: 1. Vite scripts are rewritten to script tags, in the other project these are passed in as `head` in development mode (and head is empty in production) but this makes EntryServer's default export bigger and requires propagating this development snippet through the tree, which feels wrong. 2. We write HTML to the stream manually. My suggestion is to make the entire document part of the HTML tree. See the explanation and motivation on line 73-100 in EntryServer.res.
Vite already helps us transform dynamic imports for our ReScript modules (for which we don't know the name of the .mjs or .bs.js or .cjs file when writing the ReScript code) to imports of the proper JS files. However, in some places we also want to reference the file directly, for example to be able to generate a `<script>` tag ourselves without use of `import`. Up to now this relied on a runtime inspection of the ReScript build artifacts. This commit introduces a special non-existent function in ReScript which serves as a marker for a newly introduced Vite plugin code transform. This allows the code transform to replace the function invokation with the literal string pointing to the absolute path of the source file. This allows runtime code to use the ssrManifest generated by Vite to turn this file-path into an asset chunk during production or to transform it into a path relative to the Vite dev-server working directory during development.
This moves from the marker function to a very specific object property name. This follows what was done in: - zth/rescript-relay#369 - #47 - #48 This reimplements the changes of those last two R3 PRs on top of the work done to turn server.mjs into ReScript code. A few changes are made with respect to the original PRs. *Vite Plugin* - I did not alter the user’s config. The rollup option not working with the `SSR` option is not caused by our plugin, so it shouldn’t be fixed in our plugin. It’s a user configuration error. Altering the config without the user knowing may confuse them. - Removed `production` check in `transform` this allows using the Vite generated `ssr-manifest.json` - Removed the `Rescript` check in `transform`, our regex should be fast enough that this doesn’t matter and it ensures our plugin works in codebases or pipelines that strip this header. - Removed tracking of `didReplace` since `magic-string` does this for us. *EntryServer.res* - Omitted asset emitting change for RelayRouter since the function has a different shape than RelaySSRUtils.AssetRegisterer. This was because `eagerPreloadFn` was not available in the function the router got. Ideally when we bring this back we only require the consuming developer to create a single preload handler on top of our transformation stream. *package.json* - It looks like the introduction of SSR in production mode has also exposed the bug in Relay’s faulty exports (or ReScript/Vite’s handling of them) here. - Upgrade `history` to 5.3.0 which is required for ESM support.
This was used for some ReScript file loading during dev SSR but that functionality is now entirely contained within the Vite plugin.
Re-introduces the change from 1de74d8 which works now that we have a way to reliably write asset names of our chunks into the compiled code.
There's no reason to await the route renderers of our initial routes now that suspense and preloading works properly.
0ce6211
to
4adc80b
Compare
We use the plugin to work around a bug in Vite 2 which has been fixed in Vite 3 that breaks the SSR build if the manualChunks Rollup config is used. See vitejs/vite#8836
The __$rescriptChunkName__ is a non-public identifier used to bridge the gap between the ReScript and JavaScript world. It's public API is `chunk` within ReScript and it's not intended to be used from a non-ReScript codebase.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The individual commits contain most of the explanation of what is happening.
The goal was to turn
server.mjs
into aServer.res
file so that we have ReScript across the board. In so doing I also wanted to limit the needed API surface forEntryServer.res
, making the thing that handles "Setting up a webserver" and "Turning a request into a React response" two separate things. Doing so should make it easier to support different webservers such as Fastify or different environments such as Cloudflare workers. It also more clearly encapsulates what is scaffolding and what is actually React related. This should make moving things around in the router easier too.While doing so asset preloading was something that was most spread out and as such got quite a bit of attention. As a result we now have asset preloading working (although not yet for the assets of the initial route match, only assets emitted by React) in both development and production for both SSR and CSR.