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

v1.19.0 removes build.dev, breaking logDevReady() in Cloudflare #6891

Closed
1 task done
huw opened this issue Jul 20, 2023 · 9 comments · Fixed by #6912
Closed
1 task done

v1.19.0 removes build.dev, breaking logDevReady() in Cloudflare #6891

huw opened this issue Jul 20, 2023 · 9 comments · Fixed by #6912

Comments

@huw
Copy link
Contributor

huw commented Jul 20, 2023

What version of Remix are you using?

v1.19.0

Are all your remix dependencies & dev-dependencies using the same version?

  • Yes

Steps to Reproduce

(Firstly @pcattori I f**ing love the new dev server and feel like this is as great a place as any to show my appreciation. It’s hard work to keep this compatible with all the different runtimes and other frameworks have just given up on trying—I love that you haven’t!)

This one is fairly simple, I think it’s just an oversight. The Cloudflare template includes:

if (build.dev) {
logDevReady(build);
}

But as of v1.19.0, build.dev no longer exists (blame). As such, my live reloads never trigger and I have a type error when I compile.

In the meantime, I’m gating on process.env.NODE_ENV, which I’ve been injecting into my Workers environment anyway.

Expected Behavior

The CLI should print the build hash to the console, triggering a live reload.

Actual Behavior

The CLI does not print the build hash to the console and live reloads don’t happen.

@pcattori
Copy link
Contributor

build.dev was an implementation detail that shouldn't have leaked into user-land.
I think the proper solution is to use whatever mechanism is native to your runtime to detect if running in development or not. Not an expert with CF, so not sure what the equivalent would be.

Workaround is to use process.env.NODE_ENV (which you'll have to inject/define for non-Node runtimes) or use build.assets.hmr, though that is similarly an implementation detail that just happens to still exist.

@pcattori
Copy link
Contributor

Firstly @pcattori I f**ing love the new dev server and feel like this is as great a place as any to show my appreciation.

@huw you made my morning 😁

@huw
Copy link
Contributor Author

huw commented Jul 21, 2023

@pcattori I was slightly mistaken; since server.ts is part of the Remix build it will automatically have process.env.NODE_ENV injected. As such, I’ve submitted #6912 to use that by default in the template.

@Ponjimon
Copy link

What about process not being defined and thus causing TypeScript to error in the IDE?

Cannot find name 'process'. Do you need to install type definitions for node? Try `npm i --save-dev @types/node`.ts(2580)
Screenshot 2023-07-23 at 18 23 15

It still runs, but my IDE shows me this error and I'm not sure if installing @types/node is the best way to fix this

@pcattori
Copy link
Contributor

pcattori commented Jul 23, 2023

@Ponjimon you can force types for process.env like so:

// server.ts

declare var process : {
  env: {
    NODE_ENV: string
  }
}

// ...rest of your server code goes here...

That way you don't need to rely on @types/node for non-Node runtimes.
This tells TS to expect process.env to be defined, but still relies on something else (i.e. Remix) providing process.env at runtime.

@Ponjimon
Copy link

Thanks, but shouldn't something like this be part of the template? It feels odd having a template that throws errors, even if it's not a runtime error

@huw
Copy link
Contributor Author

huw commented Jul 24, 2023

@Ponjimon Yeah, I’ll PR it. My bad for not testing properly 😔

@huw
Copy link
Contributor Author

huw commented Jul 24, 2023

No, my bad, Pedro is right—this change didn’t introduce process.env to that file, it just re-used it (the original definition was on the first errored line you have, in createRequestHandler).

@remix-run/cloudflare-workers (and countless other TypeScript packages) implicitly depends on @types/node, and the dependency itself has special status in the TypeScript compiler. I’m not sure how much work it would be to remove the dependency, but I have a modestly-sized Workers project and haven’t run into any major conflicts in practice. But you might be able to get away with just defining process.

@pcattori
Copy link
Contributor

I'm planning to add build.mode to the Remix server build output that will contain the mode used when the server was built by the compiler. That will be public API (and not an implementation detail), so then checking build.mode === 'development' will work across runtimes, including CloudFlare. This improvement will probably land in v2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants