-
Notifications
You must be signed in to change notification settings - Fork 273
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
Refactor: move classic compiler code #1999
Conversation
Oxygen deployed a preview of your
Learn more about Hydrogen's GitHub integration. |
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.
It looks like the classic remix example is broken. dev
, build
, and preview
all do not work.
I'm afraid this is related to the new global CLI 🤔 |
Yeah, this is happening as well in I'll try to find a workaround in another branch -- #2075 |
const {runClassicCompilerBuild} = await import( | ||
'../../lib/classic-compiler/build.js' | ||
); |
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.
Is the reason to only lazy load the classic compiler because we'll eventually deprecate and remove the classic compiler logic? If so, why do we not lazy load the other classic functionality for dev
, deploy
, and preview
?
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.
No, this was probably something I didn't refactor. We are already loading optional dependencies like @remix-run/dev
dynamically so we can import these functions statically everywhere.
@@ -564,7 +564,7 @@ Continue?`.value, | |||
outputContent`${colors.whiteBright('Building project...')}`.value, | |||
); | |||
|
|||
const build = isClassicCompiler ? runBuild : runViteBuild; | |||
const build = isClassicCompiler ? runClassicCompilerBuild : runBuild; |
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.
Worth lazy loading the classic compiler here too?
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.
Besides the mono repo setup with --diff
that's already known to be broken, all my testing looks good. Though let's try to fix diff asap. The sooner we get this merged, the more time we have for it to naturally QA too.
We detected some changes in |
dev-vite
anddev
commandsbuild-vite
andbuild
commandslib
init.test.ts
in different filesMinor fixes to--quickstart
Cleanup code related to the--styling
flag🎩 : Nothing should change in behavior, except for the minor fixes.