-
Notifications
You must be signed in to change notification settings - Fork 16
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
✨ Upgrade to Remix 2 [Spike] #395
base: main
Are you sure you want to change the base?
Conversation
As per usual, this is much harder than it should be because of weird dependency graphs and poor module support. It looks like Before this problem, I was struggling to deal with |
b4abe3f
to
d0fed86
Compare
OK, hacking around that by dropping syntax highlighting and overriding the vite version. Now it loads a page, but complains about mixing server and client code |
450c576
to
3eda3ee
Compare
OK, at this point myst-theme mostly works; I ripped out syntax highlighting (and probably broke some other things). FindingsSPA ModeThe code that introduced the reload behaviour has since been modified to support Remix V2's SPA mode. If we wanted to upgrade Remix, we could do so iff. we found a way to use SPA mode to generate our static exports. Generating an SPA is a useful output for MyST, but it is not the best-case scenario for static exports; client-side SPAs introduce the "white-flash" loading screen, and have a long-time to meaningful first paint. SSR-SPAHowever, underpinning Remix 2's SPA mode is the ViteAnother benefit of this upgrade, though, is that Remix now intends to use Vite for the server (faster, more plugins, etc). Footnotes
|
@rowanc1 / @stevejpurves do you recall #184 well enough to know whether the approach taken in https://github.com/brophdawg11/remix-ssg/tree/main/demos/ssg-spa-client-loaders would work? The reloading logic in Remix 2.9.2 is https://github.com/remix-run/remix/blob/ff06e1656108bc21244e1fd4b33ed53e22b85158/packages/remix-react/browser.tsx#L205-L217 My initial thought is - the bug is still there if we take our existing approach, so I am guessing perhaps not. I am only guessing, though; I have no understanding of why the Remix context wouldn't match the true URL. |
Given that this is a pretty complex PR (-2800 / +2000) I suggest that we try to decide whether it should get over the finish line or not, otherwise I worry it is going to become stale very quickly. @agoose77 what is the information that you need in order to decide whether this spike is worth implementing entirely, or whether it's best to pursue other strategies for improving the static site workflow? |
@choldgraf the line diff is deceptive; most of the changed lines are in the lock file. Given the rate of change in myst-theme, I think it will be fairly stable. At this point, I think the spike is close to done - the final day I've allocated to it will see whether static exports work (either with the remix server or vite). That will conclude the spike, and following feedback from the MyST team, we should have an idea of where to go next. |
@agoose77 wrt your last comment, did you look at and have conclusions around the out of the box static support with our current method? (i.e. without the transition to SSR-SPA) |
This PR is a spike to see whether we can upgrade to Remix 2, timeboxed to three days.
I will spend a few days (3) maximum in this direction before evaluating whether this can be made into a prototype or we need to rethink e.g using Next.js' static exports feature.
Context
We are currently pinned on an old version of Remix due to an infinite-reload bug for static exports. We cannot upgrade without breaking static exports (required for RTD, GitHub pages).