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(dev): Specify "paths" in require.resolve() check #5536

Closed
wants to merge 2 commits into from
Closed

fix(dev): Specify "paths" in require.resolve() check #5536

wants to merge 2 commits into from

Conversation

TooTallNate
Copy link
Contributor

@TooTallNate TooTallNate commented Feb 22, 2023

require.resolve() by default will resolve module paths relative to the __filename of the code that is invoking it (so relative to serverBareModulesPlugin.js in this case). This normally works, by coincidence, because usually @remix-run/dev is located within the project's node_modules directory.

However, we get a false positive when remix CLI is located outside of the project directory, for example when testing changes in a local checkout of this repo.

To fix, specify paths array to require.resolve() and explcitly specify the cwd to resolve from.

Before

~/remix $ node ~/Code/remix-run/remix/packages/remix-dev/dist/cli.js build
Building Remix app in production mode...
The path "@remix-run/react" is imported in app/entry.server.tsx but "@remix-run/react" was not found in your node_modules. Did you forget to install it?
The path "react-dom/server" is imported in app/entry.server.tsx but "react-dom/server" was not found in your node_modules. Did you forget to install it?
Built in 190ms

(The modules are definitely installed, I promise)

After

~/remix $ node ~/Code/remix-run/remix/packages/remix-dev/dist/cli.js build
Building Remix app in production mode...
Built in 176ms

Closes: #5535

  • Docs
  • Tests

`require.resolve()` by default will resolve module paths relative to the
`__filename` of the code that is invoking it (so relative to
`serverBareModulesPlugin.js` in this case). This normally works, by
coincidence, because usually `@remix-run/dev` is located within the project's
`node_modules` directory.

However, we get a false positive when `remix` CLI is located *outside*
of the project directory, for example when testing changes in a local
checkout of this repo:

```bash
$ node ~/path/to/remix-run/remix/packages/remix-dev/dist/cli.js build
```

To fix, specify `paths` array to `require.resolve()` and explcitly specify
the `cwd` to resolve from.
@changeset-bot
Copy link

changeset-bot bot commented Feb 22, 2023

🦋 Changeset detected

Latest commit: cb35f6c

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

This PR includes changesets to release 18 packages
Name Type
@remix-run/dev Patch
create-remix Patch
@remix-run/css-bundle Patch
remix Patch
@remix-run/architect Patch
@remix-run/cloudflare Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
@remix-run/deno Patch
@remix-run/eslint-config Patch
@remix-run/express Patch
@remix-run/netlify Patch
@remix-run/node Patch
@remix-run/react Patch
@remix-run/serve Patch
@remix-run/server-runtime Patch
@remix-run/testing Patch
@remix-run/vercel 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

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Feb 22, 2023

Hi @TooTallNate,

Welcome, and thank you for contributing to Remix!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at [email protected].

Thanks!

- The Remix team

@TooTallNate TooTallNate changed the title [remix-dev] Specify "paths" in require.resolve() check fix(dev) Specify "paths" in require.resolve() check Feb 22, 2023
@TooTallNate TooTallNate changed the title fix(dev) Specify "paths" in require.resolve() check fix(dev): Specify "paths" in require.resolve() check Feb 22, 2023
@github-actions github-actions bot added the needs-response We need a response from the original author about this issue/PR label Apr 23, 2023
@machour machour removed the needs-response We need a response from the original author about this issue/PR label Apr 29, 2023
@machour
Copy link
Collaborator

machour commented Apr 29, 2023

Hey @TooTallNate

Thank you for opening this pull request, however, it seems that both changed files got removed since then.
Do you want to adapt this PR to the new changes?

Also, we're going to have an entirely new dev server soon (https://remix.run/docs/en/1.15.0/pages/v2#dev-server).
You may want to check if this is still applicable for the new version, and discuss the change with @pcattori

@machour machour added the needs-response We need a response from the original author about this issue/PR label Apr 29, 2023
@remix-run remix-run deleted a comment from github-actions bot Apr 30, 2023
@github-actions github-actions bot removed the needs-response We need a response from the original author about this issue/PR label Apr 30, 2023
@TooTallNate
Copy link
Contributor Author

Looks like this was fixed by #6181.

@TooTallNate TooTallNate closed this May 1, 2023
@TooTallNate TooTallNate deleted the fix/node-modules-resolve-warning branch May 1, 2023 23:39
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.

remix CLI reports false positives of missing "node_modules" when invoked from a different directory
2 participants