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

[VITE] Missing dependencies after running setup vite command #1834

Closed
thomasKn opened this issue Mar 8, 2024 · 17 comments
Closed

[VITE] Missing dependencies after running setup vite command #1834

thomasKn opened this issue Mar 8, 2024 · 17 comments

Comments

@thomasKn
Copy link
Contributor

thomasKn commented Mar 8, 2024

What is the location of your example repository?

No response

Which package or tool is having this issue?

Hydrogen

What version of that package or tool are you using?

"@shopify/hydrogen": "~2024.1.3"

What version of Remix are you using?

"@remix-run/node": "^2.8.0"

Steps to Reproduce

  1. Create a new Hydrogen project using npm create @shopify/hydrogen@latest
  2. Run vite setup command npx shopify hydrogen setup vite
  3. Launch dev server npm run dev

Expected Behavior

Hydrogen app should launch correctly.

Actual Behavior

Missing dependencies:

Failed to resolve dependency: set-cookie-parser, present in 'ssr.optimizeDeps.include'
Failed to resolve dependency: cookie, present in 'ssr.optimizeDeps.include'
Failed to resolve dependency: content-security-policy-builder, present in 'ssr.optimizeDeps.include'

ReferenceError: exports is not defined

@wizardlyhel
Copy link
Contributor

Unable to reproduce on my end. Base on the error, it feels like your npm install didn't actually installed all the dependencies after setting up vite.

@thomasKn
Copy link
Contributor Author

@wizardlyhel I forgot to mention that I'm using pnpm

@frandiox
Copy link
Contributor

Oh, interesting, thanks for reporting this.
The problem is not that dependencies are not installed. They are but probably as sub-dependencies.
Therefore, Vite cannot find them properly when we tell it to optimize them. We should probably remove these dependencies from the optimization list.

I'll play a bit with this and try it with PNPM. For the time being, if you install those dependencies manually it should work.

@frandiox
Copy link
Contributor

So I've tried in different ways with PNPM and I wasn't able to reproduce this issue.
I'm using these versions:

❯ pnpm -v
8.10.2
❯ node -v
v18.17.1

What versions are you using?

@thomasKn
Copy link
Contributor Author

I created a clean new project following the steps in my first comment and still have the same dependency issue.
Here are the version I use:

❯ pnpm -v
8.15.5
❯ node -v
v21.7.1

@adityasingh-io
Copy link

I am getting the same error: ReferenceError: exports is not defined

Mine is caused by the typographic-base package, is there any fix for this yet?

@frandiox
Copy link
Contributor

frandiox commented Apr 3, 2024

I am getting the same error: ReferenceError: exports is not defined
Mine is caused by the typographic-base package, is there any fix for this yet?

That's probably unrelated to this specific issue. For the typographic-base dependency, check how we updated our demo-store to Vite: Shopify/hydrogen-demo-store#12

@frandiox
Copy link
Contributor

frandiox commented Apr 9, 2024

@thomasKn You mentioned you are using PNPM but the steps you wrote are using NPM. You are installing dependencies with PNPM, right?

I've tried again with the same steps (NPM) and then running pnpm i for the dependencies but it seems to be working.

These are the steps I tried:

  1. npm create @shopify/hydrogen@latest
    • Choose mock.shop; no CSS; routes without localization; no install deps
  2. cd into the project directory
  3. pnpm i
  4. npx shopify hydrogen setup vite
  5. npm run dev

Then it seems to work:

image

Am I doing anything wrong with the steps listed?
I'm also using pnpm 8.15.6 and Node 20+

@frandiox
Copy link
Contributor

I was able to reproduce this finally in our demo-store. The problem is not related to Hydrogen, though, so we can't fix much here I think.

The issue is that Vite tries to find dependencies to optimize in node_modules/* but PNPM moves most of the dependencies to node_modules/.pnpm/node_modules/* so Vite can't find them. More info here.

Try adding the following to your .npmrc file to force hoisting these dependencies that Vite must optimize for the worker runtime (from pnpm docs):

public-hoist-pattern[]=cookie
public-hoist-pattern[]=set-cookie-parser
public-hoist-pattern[]=content-security-policy-builder

You might need to add more dependencies, depending on what you are using in your project.

@thomasKn
Copy link
Contributor Author

Thanks @frandiox

I added the .npmrc file and I can confirm this removes the Failed to resolve dependency errors.

Although I still have the ReferenceError: exports is not defined error unless I install the cookie dependency.

ReferenceError: exports is not defined
    at /node_modules/.pnpm/cookie@0.6.0/node_modules/cookie/index.js?v=20f76387:15:1

@frandiox
Copy link
Contributor

Are you adding public-hoist-pattern[]=cookie in your .npmrc? If you do that, remove node_modules and pnpm i, you should already see node_modules/cookie without installing cookie directly 🤔

@thomasKn
Copy link
Contributor Author

Yeah sorry I didn't try to remove node_modules. It works perfectly now, thanks!

@tomglaize
Copy link

@frandiox This is still an issue for us unfortunately.

For context, we're using a PNPM monorepo. There's another (non-Hydrogen) Remix Vite app in the same monorepo that works without any issue (and uses many of the same deps), so I suspect these errors are being introduced by the hydrogen and oxygen Vite plugins.

It's like a game of whack-a-mole. We were able to get rid of the errors for cookie, set-cookie-parser and content-security-policy-builder by installing them in the app (prefer to avoid playing around with hoisting), but then started getting errors for typographic-base. We removed that dep from the app entirely, and then got errors for remix-routes (a package we also use in the other Remix Vite app in the monorepo without any issue). After removing remix-routes from the app we started getting errors for use-sync-external-store (see below), and I expect that if we fix that we'll just get another error.

ReferenceError: module is not defined
    at /code/node_modules/.pnpm/use-sync-external-store@1.2.0_react@18.3.1/node_modules/use-sync-external-store/shim/with-selector.js:6:3
    at Object.runViteModule (code/node_modules/.pnpm/@shopify+mini-oxygen@3.0.2_vite@5.2.11/node_modules/@shopify/mini-oxygen/dist/vite/worker-entry.js:1181:17)
    at ViteRuntime.directRequest (code/node_modules/.pnpm/@shopify+mini-oxygen@3.0.2_vite@5.2.11/node_modules/@shopify/mini-oxygen/dist/vite/worker-entry.js:1026:78)
    at ViteRuntime.cachedRequest (code/node_modules/.pnpm/@shopify+mini-oxygen@3.0.2_vite@5.2.11/node_modules/@shopify/mini-oxygen/dist/vite/worker-entry.js:949:28)
    at request (code/node_modules/.pnpm/@shopify+mini-oxygen@3.0.2_vite@5.2.11/node_modules/@shopify/mini-oxygen/dist/vite/worker-entry.js:976:128)
    at /@fs/code/node_modules/.pnpm/zustand@4.5.2_@types+react@18.3.1_immer@10.1.1_react@18.3.1/node_modules/zustand/esm/index.mjs?v=2603acb1:3:31
    at Object.runViteModule (code/node_modules/.pnpm/@shopify+mini-oxygen@3.0.2_vite@5.2.11/node_modules/@shopify/mini-oxygen/dist/vite/worker-entry.js:1181:11)
    at ViteRuntime.directRequest (code/node_modules/.pnpm/@shopify+mini-oxygen@3.0.2_vite@5.2.11/node_modules/@shopify/mini-oxygen/dist/vite/worker-entry.js:1026:60)
    at ViteRuntime.cachedRequest (code/node_modules/.pnpm/@shopify+mini-oxygen@3.0.2_vite@5.2.11/node_modules/@shopify/mini-oxygen/dist/vite/worker-entry.js:950:79)
    at /app/lib/app-state.tsx:3:31

@frandiox
Copy link
Contributor

I agree it's frustrating but this is really an issue with the dependencies. They are in CJS, which means they are only compatible with Node.js. Oxygen is a different runtime so you need to tell Vite to do its magic before loading the code in Oxygen.

That said, I'm trying to add here a tool to do all this job automatically: #2106
Hope to release it soon if it passes the reviews.

@tomglaize
Copy link

I agree it's frustrating but this is really an issue with the dependencies. They are in CJS, which means they are only compatible with Node.js. Oxygen is a different runtime so you need to tell Vite to do its magic before loading the code in Oxygen.

The Oxygen dev runtime is based on workerd, right? The Remix template for Cloudflare Workers works fine with PNPM without any hoisting and without the Cloudflare Vite plugin adding any entries to optimizeDeps. Adding a CJS-only dep like cookie to the server bundle in the template works fine as well, so it seems like workerd is compatible with CJS. Is there something special about Oxygen that breaks this compatibility?

@frandiox
Copy link
Contributor

The Oxygen dev runtime is based on workerd, right?

That's right

The Remix template for Cloudflare Workers works fine with PNPM without any hoisting and without the Cloudflare Vite plugin adding any entries to optimizeDeps. Adding a CJS-only dep like cookie to the server bundle in the template works fine as well, so it seems like workerd is compatible with CJS. Is there something special about Oxygen that breaks this compatibility?

The template you mention doesn't run on workerd. The cloudflareDevProxyVitePlugin() plugin is only adding proxies for Cloudflare APIs to the Node.js handler. This means you have access to things like context.env or context.caches, but your app code still runs on Node.js during development, that's why it doesn't break with module or require.

Running on different runtimes in dev vs prod can lead to bugs harder to debug, such as new URL(...) behaving differently in production (real example). That's why we are trying to run your code on the same runtime in development instead of just polyfilling the missing worker APIs.

In fact, once running locally on workerd becomes a standard and tested Vite plugin, I believe the Remix team will switch to that instead of polyfilling, which was supposed to be a temporary patch for a bigger problem.

@tomglaize
Copy link

Got it, thanks for clarifying. Will keep an eye on your PR as this is the only blocker for us switching to Vite

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

No branches or pull requests

5 participants