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

Read Vite config in reveal/routes commands #8916

Merged
merged 5 commits into from
Feb 28, 2024

Conversation

markdalgleish
Copy link
Member

@markdalgleish markdalgleish commented Feb 28, 2024

Fixes #8845
Fixes #8881

Copy link

changeset-bot bot commented Feb 28, 2024

🦋 Changeset detected

Latest commit: 116672f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages
Name Type
@remix-run/dev Patch
create-remix Patch
remix Patch
@remix-run/architect Patch
@remix-run/cloudflare Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
@remix-run/css-bundle Patch
@remix-run/deno Patch
@remix-run/eslint-config Patch
@remix-run/express Patch
@remix-run/node Patch
@remix-run/react Patch
@remix-run/serve Patch
@remix-run/server-runtime Patch
@remix-run/testing Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Comment on lines -18 to -57
async function resolveViteConfig({
configFile,
mode,
root,
}: {
configFile?: string;
mode?: string;
root: string;
}) {
let vite = await import("vite");

// Leverage the Vite config as a way to configure the entire multi-step build
// process so we don't need to have a separate Remix config
let viteConfig = await vite.resolveConfig(
{ mode, configFile, root },
"build", // command
"production", // default mode
"production" // default NODE_ENV
);

if (typeof viteConfig.build.manifest === "string") {
throw new Error("Custom Vite manifest paths are not supported");
}

return viteConfig;
}

async function extractRemixPluginContext(viteConfig: Vite.ResolvedConfig) {
let ctx = viteConfig["__remixPluginContext" as keyof typeof viteConfig] as
| RemixPluginContext
| undefined;

if (!ctx) {
console.error(colors.red("Remix Vite plugin not found in Vite config"));
process.exit(1);
}

return ctx;
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has moved to vite/plugin.ts since the logic is now shared and not specific to the vite:build command.

Comment on lines +253 to +256
if (!ctx) {
console.error(colors.red("Remix Vite plugin not found in Vite config"));
process.exit(1);
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error used to be in extractRemixPluginContext but since it's now used in contexts where it's not guaranteed that the consumer is using Vite, extractRemixPluginContext now returns undefined if Vite and/or the Remix Vite plugin aren't found, so we handle the error here now instead.

Comment on lines +43 to +74
export async function resolveViteConfig({
configFile,
mode,
root,
}: {
configFile?: string;
mode?: string;
root: string;
}) {
let vite = await import("vite");

let viteConfig = await vite.resolveConfig(
{ mode, configFile, root },
"build", // command
"production", // default mode
"production" // default NODE_ENV
);

if (typeof viteConfig.build.manifest === "string") {
throw new Error("Custom Vite manifest paths are not supported");
}

return viteConfig;
}

export async function extractRemixPluginContext(
viteConfig: Vite.ResolvedConfig
) {
return viteConfig["__remixPluginContext" as keyof typeof viteConfig] as
| RemixPluginContext
| undefined;
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted above, these have moved from vite/build.ts.

Comment on lines +76 to +105
export async function loadVitePluginContext({
configFile,
root,
}: {
configFile?: string;
root?: string;
}) {
if (!root) {
root = process.env.REMIX_ROOT || process.cwd();
}

configFile =
configFile ??
findConfig(root, "vite.config", [
".ts",
".cts",
".mts",
".js",
".cjs",
".mjs",
]);

// V3 TODO: Vite config should not be optional
if (!configFile) {
return;
}

let viteConfig = await resolveViteConfig({ configFile, root });
return await extractRemixPluginContext(viteConfig);
}
Copy link
Member Author

@markdalgleish markdalgleish Feb 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the logic used by the reveal and routes commands. Since it's not guaranteed that consumers are using Vite, this function returns undefined if a Vite config doesn't exist or if the Remix Vite plugin isn't present. This allows us to fall back to the old Remix config.

Copy link
Contributor

@brophdawg11 brophdawg11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Gave it a quick test on my vite app and npx remix routes picked up custom routes. I also pushed a commit to this branch for npx remix reveal to reveal the simpler SPA mode server entry.

Comment on lines 101 to 102
let formatArg = flags.json ? "json" : "jsx";
let format = isRoutesFormat(formatArg) ? formatArg : RoutesFormat.jsx;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can streamline this due to the boolean and remove the isRoutesFormat utility?

Suggested change
let formatArg = flags.json ? "json" : "jsx";
let format = isRoutesFormat(formatArg) ? formatArg : RoutesFormat.jsx;
let format = flags.json ? RoutesFormat.json : RoutesFormat.jsx;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +127 to +134
{
"--config": String,
"-c": "--config",
}
: {
// Handle non Vite config commands
"-c": "--command",
}),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels slightly odd but looks like it was already there before - npx remix -c means something different depending on if you're using vite?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue was that remix vite: commands are designed to mirror the Vite CLI where -c is short for --config. This config flag now needs to work with reveal and routes too. Passing -c to these commands currently has no effect.

@@ -120,14 +120,25 @@ export async function run(argv: string[] = process.argv.slice(2)) {
"--tls-key": String,
"--tls-cert": String,

...(argv[0].startsWith("vite:") ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this PR but I noticed we don't mention anything about the vite: commands in the npx remix --help output - might be worth a follow up to add some info there?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the --help output here: #8939

@markdalgleish markdalgleish merged commit 6d32ca5 into dev Feb 28, 2024
9 checks passed
@markdalgleish markdalgleish deleted the markdalgleish/reveal-routes-vite-config branch February 28, 2024 20:22
@github-actions github-actions bot added the awaiting release This issue has been fixed and will be released soon label Feb 28, 2024
Copy link
Contributor

github-actions bot commented Mar 5, 2024

🤖 Hello there,

We just published version 2.8.1-pre.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Copy link
Contributor

github-actions bot commented Mar 7, 2024

🤖 Hello there,

We just published version 2.8.1 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions github-actions bot removed the awaiting release This issue has been fixed and will be released soon label Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SPA Mode: remix reveal generate entries files with unexpected content remix routes does not work with vite
2 participants