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

fix(vite): don't generate tasks for remix projects #22551

Merged
merged 2 commits into from May 8, 2024

Conversation

SeanSanker
Copy link
Contributor

Current Behavior

Attempting to use Remix + Vite results in a few errors. I assume this is due to the vite and remix plugins conflicting with each other.

One of which being:

Failed to process project graph.
  The "@nx/vite/plugin" plugin threw an error while creating nodes from myremixapp/vite.config.ts:
    Error: Missing "root" route file in /Users/username/work/remix-demo/app
        at Object.resolveConfig (/Users/username/work/remix-demo/node_modules/@remix-run/dev/dist/config.js:154:11)
        at updateRemixPluginContext (/Users/username/work/remix-demo/node_modules/@remix-run/dev/dist/vite/plugin.js:367:9)
        at config (/Users/username/work/remix-demo/node_modules/@remix-run/dev/dist/vite/plugin.js:598:7)

Expected Behavior

It should work just like it currently does for Remix Classic.

Related Issue(s)

#22035

PR Status

This is a very early draft. I don't think this is a good approach but it does work for me at the moment. I'm not sure exactly how we can discern a remix project from a vanilla vite project (that you can use standard tooling for) without parsing the package.json or vite.config.ts and searching for remix-specific content.

Steps to reproduce my current state

Set up a standard @nx/remix project as shown here.
Follow the instructions here from Migrating down to but NOT including Migrating a custom server.

Once I use the modified @nx/vite code provided in this PR, I'm able to run npx nx dev [app-name] successfully.

A Personal Note

I'd love to contribute more to nrwl/nx.
I'm quite a fan of Nx and use it in a few separate projects.
That being said, I don't currently have a comprehensive knowledge of its internals.
If anyone wants to give me some guidance (text-based or we can hop on a call), I'd be more than happy to contribute the rest of this myself (and other fixes).

Copy link

vercel bot commented Mar 27, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Updated (UTC)
nx-dev ⬜️ Ignored (Inspect) Visit Preview May 8, 2024 9:48am

@SeanSanker SeanSanker force-pushed the remix-vite branch 2 times, most recently from 216ad65 to 04c9195 Compare March 28, 2024 16:15
@Coly010
Copy link
Contributor

Coly010 commented Apr 4, 2024

Hi @SeanSanker :) Thanks for the contribution!

Your solution does work, however, I think it would be better and potentially easier to simply check the siblingFiles to see if a remix.config file exists. You may have to use a regex pattern for the file extension on the remix config file name (remix\.config\.(m|c)*(ts|js)+) (https://regexr.com/7ufnd)

@SeanSanker
Copy link
Contributor Author

Hi there, @Coly010!

Thank you for your reply!

I was going to go down that route, but it seems that in the new Remix Vite setup, they've deprecated the remix.config.* file in favor of vite.config.* migration example here.

From the new templates here, they don't have the remix.config.* file, but do have the vite.config.*.

I believe the remix.config.* format is now considered the "classic" compiler.
Here are the "classic" templates.

As a workaround for my other projects at the moment, I've removed the @nx/vite/plugin from nx.json and added package.json scripts for running the tasks.

What do you think?

@Coly010
Copy link
Contributor

Coly010 commented Apr 4, 2024

Yes, reading the documentation more thoroughly this does seem to be the case.

In which case the @nx/remix/plugin will not create tasks at all either, because it looks for remix.config.* files.

Your solution approach is fine, however, I don't necessarily want us to read every vite config file in the workspace when creating the project graph as this will impact performance.

Let me have a think about this and I'll get back to you 🙂

@SeanSanker
Copy link
Contributor Author

I don't necessarily want us to read every vite config file in the workspace when creating the project graph as this will impact performance

That's a very good point!

Let me have a think about this and I'll get back to you 🙂

Sounds good! Let me know if there's anything I can do to help.
I'd love to contribute further to nx (and better understand its internals). :)

@unrealsolver
Copy link
Contributor

Should it include remix stuff? like

remix({
  ignoredRouteFiles: ["**/*.css"],
})

@pcfreak30
Copy link

pcfreak30 commented Apr 8, 2024

Found a bug in this logic overall? not really clear if the vite or the remix executor is supposed to run @SeanSanker . But the remix plugin currently has:

exports.createNodes = [
    '**/remix.config.{js,cjs,mjs}',
    async (configFilePath, options, context) => {
        const projectRoot = (0, path_1.dirname)(configFilePath);
        const fullyQualifiedProjectRoot = (0, path_1.join)(context.workspaceRoot, projectRoot);
        // Do not create a project if package.json and project.json isn't there
        const siblingFiles = (0, fs_1.readdirSync)(fullyQualifiedProjectRoot);
        if (!siblingFiles.includes('package.json') &&
            !siblingFiles.includes('project.json') &&
            !siblingFiles.includes('vite.config.ts') &&
            !siblingFiles.includes('vite.config.js')) {
            return {};
        }

And spent a good of bit of time tracing why the tasks were not getting created. had to make an empty remix.config.js for detection only. I only had a vite.config.ts file. Will post if I hit anything else getting this working.

@unrealsolver
Copy link
Contributor

I (almost) migrated to Vite in @nx/remix . There are just 2 lines of executor's code. However, it is working very well. I am waiting for an info from the dev team regarding 2 options: migrate to 'vite' vs support 'classic' and 'vite' (that's would be harder, obviously)

@pcfreak30
Copy link

I (almost) migrated to Vite in @nx/remix . There are just 2 lines of executor's code. However, it is working very well. I am waiting for an info from the dev team regarding 2 options: migrate to 'vite' vs support 'classic' and 'vite' (that's would be harder, obviously)

I actually just patched this, but doing other bug hunting atm in remix-vite. will post a pnpm patch soon (its a hack).

@pcfreak30
Copy link

pcfreak30 commented Apr 9, 2024

patches/@nx__remix@18.2.3.patch

diff --git a/src/plugins/plugin.js b/src/plugins/plugin.js
index 6850cb0144d87efdbcb10ac4274532adcc16be6c..f01fbca3078dd9844585223c571c0ecbe9a070ec 100644
--- a/src/plugins/plugin.js
+++ b/src/plugins/plugin.js
@@ -80,7 +80,7 @@ function buildTarget(buildTargetName, projectRoot, buildDirectory, assetsBuildDi
                 : ['default', '^default']),
         ],
         outputs: [serverBuildOutputPath, assetsBuildOutputPath],
-        command: 'remix build',
+        command: 'remix vite:build',
         options: { cwd: projectRoot },
     };
 }

Not sure what the executor module does vs plugin as it seems to call a sub process and do things differently. but replacing the command here, then deleting the .nx cache folder works.

@pcfreak30
Copy link

Oh and I also have the PR here as a patch:

patches/@nx__vite@18.2.3.patch

diff --git a/src/plugins/plugin.js b/src/plugins/plugin.js
index c67882a02816092f7df440ec571a9899996fb28d..f07d0e37933533b047dc8e24039b5d5b079fbfaf 100644
--- a/src/plugins/plugin.js
+++ b/src/plugins/plugin.js
@@ -61,15 +61,26 @@ const createNodes = [
         };
     }
 ];
+const remixViteImportPattern = /import\s+.*\s+from\s+['"]@remix-run\/dev['"]/s;
 async function buildViteTargets(configFilePath, projectRoot, options, context) {
     const absoluteConfigFilePath = (0, _devkit.joinPathFragments)(context.workspaceRoot, configFilePath);
+    if ((0, _fs.existsSync)(absoluteConfigFilePath)) {
+        const fileContent = (0, _fs.readFileSync)(absoluteConfigFilePath, 'utf-8');
+
+        // If the config file imports from '@remix-run/dev', we skip creating targets
+        // as it should be handled by the @nx/remix plugin
+        if (remixViteImportPattern.test(fileContent)) {
+            return {};
+        }
+    }
+
     // Workaround for the `build$3 is not a function` error that we sometimes see in agents.
     // This should be removed later once we address the issue properly
     try {
         const importEsbuild = ()=>new Function('return import("esbuild")')();
         await importEsbuild();
     } catch (e) {
-    // do nothing
+        // do nothing
     }
     const { resolveConfig } = await (0, _executorutils.loadViteDynamicImport)();
     const viteConfig = await resolveConfig({

@Coly010
Copy link
Contributor

Coly010 commented Apr 9, 2024

As mentioned here: #22757 (comment)
The full work for supporting Remix Vite is a large-scale piece spanning many areas.

This PR is focused on preventing Vite from creating erroneous tasks for Remix projects in Nx workspaces that have manually migrated to Vite.

We should keep this PR on track.

@pcfreak30
Copy link

As mentioned here: #22757 (comment) The full work for supporting Remix Vite is a large-scale piece spanning many areas.

This PR is focused on preventing Vite from creating erroneous tasks for Remix projects in Nx workspaces that have manually migrated to Vite.

We should keep this PR on track.

Woops, sorry I wasn't aware of that PR 😅

@Coly010 Coly010 force-pushed the remix-vite branch 3 times, most recently from 07983aa to bf9b198 Compare April 15, 2024 12:38
@Coly010 Coly010 marked this pull request as ready for review May 8, 2024 10:45
@Coly010 Coly010 requested a review from a team as a code owner May 8, 2024 10:45
@Coly010 Coly010 requested a review from ndcunningham May 8, 2024 10:45
@Coly010 Coly010 merged commit 1502433 into nrwl:master May 8, 2024
6 checks passed
FrozenPandaz pushed a commit that referenced this pull request May 9, 2024
<!-- Please make sure you have read the submission guidelines before
posting an PR -->
<!--
https://github.com/nrwl/nx/blob/master/CONTRIBUTING.md#-submitting-a-pr
-->

<!-- Please make sure that your commit message follows our format -->
<!-- Example: `fix(nx): must begin with lowercase` -->

## Current Behavior
<!-- This is the behavior we have today -->
Attempting to use Remix + Vite results in a few errors. I assume this is
due to the vite and remix plugins conflicting with each other.

One of which being:
```
Failed to process project graph.
  The "@nx/vite/plugin" plugin threw an error while creating nodes from myremixapp/vite.config.ts:
    Error: Missing "root" route file in /Users/username/work/remix-demo/app
        at Object.resolveConfig (/Users/username/work/remix-demo/node_modules/@remix-run/dev/dist/config.js:154:11)
        at updateRemixPluginContext (/Users/username/work/remix-demo/node_modules/@remix-run/dev/dist/vite/plugin.js:367:9)
        at config (/Users/username/work/remix-demo/node_modules/@remix-run/dev/dist/vite/plugin.js:598:7)
```

## Expected Behavior
<!-- This is the behavior we should expect with the changes in this PR
-->
It should work just like it currently does for Remix Classic.

## Related Issue(s)
<!-- Please link the issue being fixed so it gets closed when this is
merged. -->
#22035

## PR Status
This is a very early draft. I don't think this is a good approach but it
does work for me at the moment. I'm not sure exactly how we can discern
a remix project from a vanilla vite project (that you can use standard
tooling for) without parsing the `package.json` or `vite.config.ts` and
searching for remix-specific content.

## Steps to reproduce my current state
Set up a standard @nx/remix project as shown
[here](https://nx.dev/recipes/react/remix).
Follow the instructions
[here](https://remix.run/docs/en/main/future/vite#migrating) from
`Migrating` down to but NOT including `Migrating a custom server`.

Once I use the modified @nx/vite code provided in this PR, I'm able to
run `npx nx dev [app-name]` successfully.

## A Personal Note
I'd love to contribute more to nrwl/nx.
I'm quite a fan of Nx and use it in a few separate projects.
That being said, I don't currently have a comprehensive knowledge of its
internals.
If anyone wants to give me some guidance (text-based or we can hop on a
call), I'd be more than happy to contribute the rest of this myself (and
other fixes).

---------

Co-authored-by: Colum Ferry <cferry09@gmail.com>
(cherry picked from commit 1502433)
Copy link

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants