Skip to content
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

Merged
merged 20 commits into from May 7, 2024
Merged

Conversation

frandiox
Copy link
Contributor

@frandiox frandiox commented Apr 15, 2024

  • Merge dev-vite and dev commands
  • Merge build-vite and build commands
  • Move classic compiler code to lib
  • Split init.test.ts in different files
  • Minor fixes to --quickstart
  • Cleanup code related to the --styling flag

🎩 : Nothing should change in behavior, except for the minor fixes.

@frandiox frandiox requested a review from a team April 15, 2024 12:38
Copy link
Contributor

shopify bot commented Apr 15, 2024

Oxygen deployed a preview of your fd-move-classic-compiler-code branch. Details:

Storefront Status Preview link Deployment details Last update (UTC)
vite ✅ Successful (Logs) Preview deployment Inspect deployment May 7, 2024 4:47 AM
custom-cart-method ❌ Error (Logs) - Inspect deployment May 7, 2024 4:49 AM
subscriptions ✅ Successful (Logs) Preview deployment Inspect deployment May 7, 2024 4:47 AM
third-party-queries-caching ✅ Successful (Logs) Preview deployment Inspect deployment May 7, 2024 4:47 AM
skeleton ✅ Successful (Logs) Preview deployment Inspect deployment May 7, 2024 4:47 AM
optimistic-cart-ui ✅ Successful (Logs) Preview deployment Inspect deployment May 7, 2024 4:47 AM

Learn more about Hydrogen's GitHub integration.

@frandiox frandiox mentioned this pull request Apr 19, 2024
Copy link
Contributor

@blittle blittle left a 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.

@frandiox
Copy link
Contributor Author

frandiox commented May 1, 2024

I'm afraid this is related to the new global CLI 🤔

@frandiox
Copy link
Contributor Author

frandiox commented May 2, 2024

Yeah, this is happening as well in main branch and it's related to the new CLI 3.59.2 being incompatible with --diff: https://github.com/Shopify/cli/blob/9f66320f3d56f69e3d77d173a0eca31cc7fc9f96/packages/cli/src/hooks/init.ts#L10

I'll try to find a workaround in another branch -- #2075

@frandiox frandiox requested a review from blittle May 2, 2024 04:02
Comment on lines 57 to 59
const {runClassicCompilerBuild} = await import(
'../../lib/classic-compiler/build.js'
);
Copy link
Contributor

@blittle blittle May 6, 2024

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?

Copy link
Contributor Author

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;
Copy link
Contributor

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?

Copy link
Contributor

@blittle blittle left a 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.

Copy link
Contributor

github-actions bot commented May 7, 2024

We detected some changes in packages/*/package.json or packages/*/src, and there are no updates in the .changeset.
If the changes are user-facing and should cause a version bump, run npm run changeset add to track your changes and include them in the next release CHANGELOG.
If you are making simple updates to examples or documentation, you do not need to add a changeset.

@frandiox frandiox merged commit a9c6de0 into main May 7, 2024
13 checks passed
@frandiox frandiox deleted the fd-move-classic-compiler-code branch May 7, 2024 04:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants