[WEB-4086] Port Next projects to Vite 4#2491
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
fc2e338 to
97c2076
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
09ad44b to
ca2cede
Compare
| outline: none; | ||
| transition: all 0.2s ease-in-out; | ||
| } | ||
| @import '../../../styles.css'; |
There was a problem hiding this comment.
This is an indicator of a change common throughout this PR - project stylesheets have been de-duplicated, with examples/styles.css housing all the common styles, and these ones only importing and adding unique styles.
In the case of no unique styles, the React apps import from examples/styles.css directly. The JS ones have a one line file with an input since index.html can't seem to reach back beyond the directory it's in.
There was a problem hiding this comment.
@jamiehenson how will Sandpack handle this? maybe we can include the file and rewrite the import statement while passing it in?
There was a problem hiding this comment.
@kennethkalmer I've bitten the bullet and brought all the stylesheets back in for the react projects, also relocating them into /src which sandpack more natively expects.
Squashed and cherry picked this into the sandpack branch, it works 👍 (thankfully, the child postcss/vite/tw/ts configs are minimal enough to be handled by sandpack's defaults it seems - so we can maintain a compromise between de-duplication in the repo and stuff that works downstream)
There was a problem hiding this comment.
One dumb thing I was bashing my head against though is that Sandpack hates Tailwind if it's not brought in as an external script - but that's a downstream change, not for here
|
|
||
| export default defineConfig({ | ||
| ...baseConfig, | ||
| envDir: '../../', |
There was a problem hiding this comment.
This was a more vite-y approach than dotenv
| @@ -0,0 +1,84 @@ | |||
| import React, { useState, useEffect } from 'react'; | |||
There was a problem hiding this comment.
I wouldn't look too closely at the auth-* projects. They're not in scope to support just yet.
| import * as Ably from 'ably'; | ||
| import { ChatClient, PresenceEvent, PresenceMember, AllFeaturesEnabled } from '@ably/chat'; | ||
| import { faker } from '@faker-js/faker'; | ||
| import minifaker from 'minifaker'; |
There was a problem hiding this comment.
minifaker was brought in purely because faker blows up sandpack for reasons long and arduous
| "react-dom": "^18", | ||
| "react-icons": "^5.4.0", | ||
| "ts-node": "^10.9.2", | ||
| "react-router-dom": "^6.22.2", |
There was a problem hiding this comment.
Brought in react router as the most general example of a routing solution for react, nothing fancy
ca2cede to
5ebff79
Compare
kennethkalmer
left a comment
There was a problem hiding this comment.
From my side I think the CSS issue was the only one I spotted so happy to see it is resolved. I think we'll find any other snakes in the grass when packaging them up in sandpack
Excellent conducting @jamiehenson!
| @@ -36,3 +36,35 @@ yarn-error.log* | |||
| next-env.d.ts | |||
|
|
|||
| .env.local | |||
|
|
|||
There was a problem hiding this comment.
It's not fresh. I can search and replace out the next references in the project gitignores
There was a problem hiding this comment.
Hm, even though stale may be better to keep in actually, for the moment. Removing the entries surfaces lots of next stuff that's laying in wait (useless stuff like cache data etc), don't want accidental commits of this stuff elsewhere
5ebff79 to
9330130
Compare
9330130 to
cbd8a8f
Compare
Sandpack is a fussy master.
Unfortunately we can't use Next projects with it, at least with the version of Sandpack we have access to, due to its server-rendering foundations. Thus, it's necessary to rewrite the Next projects to something else that's more clientside, and an obvious candidate for that due to its usage elsewhere here and the general deprecation of
create-react-app, is Vite. Though, naturally, Sandpack demands an older version of Vite (4) instead of anything more modern (5, 6).This PR is a semi-automated rewrite of those projects, and I've also taken the opportunity to further the initial config de-duplication work to make further use of workspaces and reduce the file count down. It also swaps out
fakerforminifaker, which is objectively inferior but works with Sandpack.I've tested all of the non-auth react projects locally and they work, with the exception of
spaces-live-cursors-javascriptwhich has an ongoing issue not related to this migration. We'll have to hold back on the auth projects and release them with some backend infrastructure in place at a later date.For @GregHolmes:
Project-wise the general approach I/the bot took was to consolidate the
app/page.tsxandapp/layout.tsxinto oneApp.tsxcomponent, and replicate multi-page apps withreact-router