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

In the module resolution playground app the react import works during dev but not build #82

Closed
dario-piotrowicz opened this issue Dec 2, 2024 · 6 comments · Fixed by #107
Assignees

Comments

@dario-piotrowicz
Copy link
Contributor

If you run the module resolution playground app in dev mode everything seems to be working fine

But if you run it in preview mode and fetch from /third-party/react you get the following error:
Screenshot 2024-12-02 at 10 19 31

(which is something we've seen in the past: #22)

Something is not adding up here to me, because in the module resolution app I don't see us opting in nodejs_compat anywhere, so should this issue not be present for dev as well?

Additionally if I add

compatibility_flags = ['nodejs_compat']

to the wrangler.toml file, then both for dev and build the app breaks completely erroring that the unenv/runtime/node/process/$cloudflare module can't be found

@jamesopstad
Copy link
Contributor

Additionally if I add

compatibility_flags = ['nodejs_compat']

to the wrangler.toml file, then both for dev and build the app breaks completely erroring that the unenv/runtime/node/process/$cloudflare module can't be found

You need to add unenv as a dependency if you use nodejs_compat in one of the playgrounds. I remember @petebacondarwin saying that this is just a quirk of it being in a monorepo with the plugin package. We should verify that this isn't necessary when using the published package.

@dario-piotrowicz
Copy link
Contributor Author

This is the problematic piece of code: react code

Without nodejs_compat (which I don't think should be needed for react)

In dev, in the pre-bundled react file process.env.NODE_ENV === 'production' is already resolved for false:
Screenshot 2024-12-05 at 11 11 05

But in the built output it is not:
Screenshot 2024-12-05 at 11 14 48

which explains why we are encountering the problem for build but not for dev

So what I need to do now is understanding why rollup doesn't resolve process.env.NODE_ENV === 'production' to true, as I assume it should do 🤔

@dario-piotrowicz
Copy link
Contributor Author

Ok I think this sums up the rollup issue quite well: rollup/rollup#487

@dario-piotrowicz
Copy link
Contributor Author

Ok this seems to be working quite well:
Screenshot 2024-12-05 at 11 36 03

With it the react development code does get treeshaken away! 🚀

I think this might be the appropriate solution (I can create a PR with this as a followup to #72)

@jamesopstad
Copy link
Contributor

Nice! This feels like a weird one because you would expect people to run into this a lot.

@dario-piotrowicz
Copy link
Contributor Author

Nice! This feels like a weird one because you would expect people to run into this a lot.

yeah I totally agree... 😕

I even wonder if something like this should be upstreamed in vite itself, isn't replacing process.env.NODE_ENV on build a very common behavior? it feels very weird to me that rollup does not do that 😕

@dario-piotrowicz dario-piotrowicz changed the title In the module resolution playground app something is wrong in regards to nodejs_compat In the module resolution playground app the react import works during dev but not build Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment