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

Broken production with Metro bundler (ReferenceError) #736

Closed
zoontek opened this issue Apr 22, 2020 · 12 comments · Fixed by #737
Closed

Broken production with Metro bundler (ReferenceError) #736

zoontek opened this issue Apr 22, 2020 · 12 comments · Fixed by #737
Labels
bug 🐛 Oh no! A bug or unintented behaviour.

Comments

@zoontek
Copy link
Contributor

zoontek commented Apr 22, 2020

Hi 👋

On the latest version, a modification (somewhere around here) broke the metro bundler in production mode (and possibly other production apps? It's not tied to minification, but I didn't tried on other bundlers).

urql version: 1.9.7

Steps to reproduce

  1. Create a new React Native project
  2. Add [email protected], wrap your app in a Provider with an urql client
  3. Build your app for production
  4. Starts your app. It will crash at client init.

Expected behavior

No JS ReferenceError.

Actual behavior

For some reason, the variable is not declared.

Debugging

In order to spot the issue, I created a server to serve the unminified production bundle to my debug application. Create a file at project root:

// server.js

const express = require("express");
const app = express();

app.use((req, res, next) => {
  console.log(req.url);
  res.setHeader("Content-Type", "application/javascript");
  next();
});

app.use("/", express.static("."));
app.use("/assets", express.static("."));
app.get("/status", (_req, res) => {
  res.send("packager-status:running");
});

app.listen(8081, () => {
  console.log("Serving directory as localhost:8081");
});

Build a bundle with:

yarn react-native bundle --entry-file index.js --bundle-output index.bundle --platform ios --dev false --minify false

Then run the server.js file to fake metro bundler, it will be easier to debug.

Fix

The missing declaration is f:

Screenshot 2020-04-22 at 13 57 36

If I add it manually to the bundle, it works:

Screenshot 2020-04-22 at 13 57 58

@zoontek zoontek added the bug 🐛 Oh no! A bug or unintented behaviour. label Apr 22, 2020
@zoontek zoontek changed the title Broken production issue with Metro bundler (ReferenceError) Broken production with Metro bundler (ReferenceError) Apr 22, 2020
@JoviDeCroock
Copy link
Collaborator

Hey would you mind doing yarn list --pattern "urql" and pasting the output here please

@zoontek
Copy link
Contributor Author

zoontek commented Apr 22, 2020

This one breaks:

yarn list v1.22.4
├─ @graphql-codegen/[email protected]
├─ @urql/[email protected]
└─ [email protected]

This one works:

yarn list v1.22.4
├─ @graphql-codegen/[email protected]
├─ @urql/[email protected]
└─ [email protected]

@JoviDeCroock
Copy link
Collaborator

JoviDeCroock commented Apr 22, 2020

Hmm, that's an odd one. In both builds this seems defined. The line you're showing in your screenshot also doesn't seem to exist.

ESM - CJS

Seems to work with CRA and Next.js

@zoontek
Copy link
Contributor Author

zoontek commented Apr 22, 2020

Yes that's extremely weird. I suppose Metro rewrite code in an odd way.

@JoviDeCroock
Copy link
Collaborator

I wonder what could be influencing this, is it miss-behaving in inlining the code it removed due to the replace of process.env 🤔

@zoontek
Copy link
Contributor Author

zoontek commented Apr 22, 2020

Wait, I got it! You checked the wrong version (my fault, I shouldn't have posted both)

f is declared here: https://www.runpkg.com/?@urql/[email protected]/dist/urql-core.js#383 (in "production" !== process.env.NODE_ENV) and reused here: https://www.runpkg.com/?@urql/[email protected]/dist/urql-core.js#396. So it's no tied to metro, each bundler using urql-core.js will break, I think.

@JoviDeCroock
Copy link
Collaborator

JoviDeCroock commented Apr 22, 2020

It works in other bundlers the problem seems to be that metro does not account for the code blocks it snips out, they must be doing this as post-processing.

We'll do a PR to ensure stability on metro, hang tight.

@zoontek
Copy link
Contributor Author

zoontek commented Apr 22, 2020

Maybe they just remove the conditional code block without taking care of variables defined inside.

@kitten
Copy link
Member

kitten commented Apr 22, 2020

Can you try using the new build from the PR above that CodeSandbox CI provides please? That should fix the issue and if so we can publish the verified fix.

If you're using Yarn you can just add a resolution to your package.json

  "resolutions": {
    "@urql/core": "https://pkg.csb.dev/FormidableLabs/urql/commit/48f86fd3/@urql/core"
  },

@zoontek
Copy link
Contributor Author

zoontek commented Apr 22, 2020

@kitten It works! hoist_vars did the trick 🎉

@kitten
Copy link
Member

kitten commented Apr 22, 2020

This should be released in @urql/[email protected], so a quick yarn upgrade @urql/core should fix the issue 👍 https://github.com/FormidableLabs/urql/releases/tag/%40urql%2Fcore%401.11.5

@zoontek
Copy link
Contributor Author

zoontek commented Apr 22, 2020

Thanks, that was fast!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Oh no! A bug or unintented behaviour.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants