Shopify / hydrogen

Hydrogen lets you build faster headless storefronts in less time, on Shopify.
https://hydrogen.shop
MIT License
1.24k stars 246 forks source link

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

Closed thomasKn closed 2 months ago

thomasKn commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

frandiox commented 3 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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: https://github.com/Shopify/hydrogen-demo-store/pull/12

frandiox commented 2 months ago

@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 commented 2 months ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

tomglaize commented 1 month ago

@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 commented 1 month ago

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: https://github.com/Shopify/hydrogen/pull/2106 Hope to release it soon if it passes the reviews.

tomglaize commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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