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

Add support for edge runtimes #2

Open
slowbaker opened this issue Nov 26, 2022 · 9 comments · May be fixed by #15
Open

Add support for edge runtimes #2

slowbaker opened this issue Nov 26, 2022 · 9 comments · May be fixed by #15

Comments

@slowbaker
Copy link

Znv causes an error if it's run "on the edge" (for example on Vecel's Edge Runtime).

error - Error: The edge runtime does not support Node.js 'tty' module.
Learn More: https://nextjs.org/docs/messages/node-module-in-edge-runtime

The tty module is used by colorette.

@lostfictions
Copy link
Owner

lostfictions commented Nov 28, 2022

Thanks for the report! I'm not using any Node-incompatible runtimes and don't have plans to do so... that said, colorette is an extremely small module, so it might be possible to inline it and adapt it to unblock this use-case.

On the other hand, colorette already does feature detection and can run fine in non-Node environments like the browser via polyfill. (For example, Webpack 4 polyfills tty with a simple module whose isatty() function returns false; the same polyfill can be manually included for Webpack 5+.) You might be able to use such a polyfill for your case.

Do you know if conditionally requiring tty (by wrapping it in a try-catch, say) would fix it? It's a little annoying to do this sort of thing when using TypeScript and it's not really forward-compatible with ES Modules, but I don't mind adding it.

Incidentally, other colour modules seem to suffer from the same issue. chalk also does feature detection by importing tty, for example.

@dctalbot
Copy link

@lostfictions FWIW I think supporting browsers would be a huge value-add for this library, especially given that zod runs in browsers. This issue is blocking adoption of znv in my org, so we are having to re-implement the preprocessors that are maintained in this repo. Are you open to accepting PRs for this work?

@dctalbot
Copy link

To give a specific example of how this would be used: webpack plumbs environment variables into app code using Webpack.DefinePlugin. Then in the app code, you would just use znv.parse(process.env) to get typed environment variables while developing. You could use the same znv.parse(process.env) as a build step on the server and exit the process if there are any errors.

@lostfictions
Copy link
Owner

lostfictions commented Aug 16, 2023

@dctalbot Would gratefully accept PRs for this!

One asterisk about solutions like Webpack.DefinePlugin is that the suggested way of using it is to configure it to replace instances of the literal string process.env.MY_VAR, rather than fully replacing process.env or process. (See the yellow "Warning" callout box in the Webpack documentation.) Some tools like Next.js have gone a step further and enforced this convention (see the code sample following "dynamic lookups will not be inlined..." in the Next.js documentation.) That means that if you simply call parse(process.env), these variables might not be present as expected.

So I definitely understand the value-add and that there are good use cases, but I'm worried that this will also become a common source of confusion. I'm happy to support browsers on a best-effort basis, but I'd like any PR to make sure these caveats are well-documented in the README. Does that sound reasonable?

Finally, if this work can be done by removing colorette as a dependency (by adapting and/or inlining it, say) and ensuring forward-compatibility with ES Modules (#8, which I'd like to help get landed soon) that'd be ideal.

@dctalbot
Copy link

Nice, I agree supporting ES modules is more important. Thanks for the note of caution - it's funny you mention that because I had the same realization/concern when working with this code recently. create-react-app replaces process.env (see here), and this is the code my org inherited. It's unfortunately one of many dubious decisions CRA makes despite its widespread adoption.

Instead of clobbering process.env, I suppose one could use a new global e.g.

  const stringified = {
    'MY_APP_ENV_CONFIG': Object.keys(raw).reduce((env, key) => {
      env[key] = JSON.stringify(raw[key]);
      return env;
    }, {}),
  };

In app code:

const env = znv.parse(MY_APP_ENV_CONFIG)

On server:

const env = znv.parse(process.env)

IMO supporting other runtimes is still worthwhile, but agreed that any examples in documentation should steer clear of this pitfall.

@BowlingX
Copy link
Contributor

BowlingX commented Aug 21, 2023

@slowbaker Could you try the latest version of bowlingx-znv (0.4.5) ? I added a check there for the nextjs runtime.
I will create a new PR for that when the ES modules are merged.

Example (with nextjs app router) and edge route handler:

import { parseEnv } from 'bowlingx-znv'
import { NextResponse } from 'next/server'
import { z } from 'zod'

export const runtime = 'edge'

const testConfig = parseEnv(process.env, {
    NODE_ENV: z.string()
})

export async function GET(request: Request) {
    return NextResponse.json({ env: testConfig.NODE_ENV })
}

https://codesandbox.io/p/sandbox/relaxed-bogdan-cz9qvk?file=/app/api/route.ts:7,43

@lostfictions
Copy link
Owner

I've just opened #15 which should add edge runtime support via a separate entrypoint -- could folks here who requested this feature take a look and see if it works for you?

I've published it to npm under the next tag, so you can npm i/pnpm add/yarn add znv@next to try it out.

@bastiankistner
Copy link

I am seeing the following now

../../node_modules/znv/dist/util/tty-colors.js
A Node.js module is loaded ('tty' at line 24) which is not supported in the Edge Runtime.
Learn More: https://nextjs.org/docs/messages/node-module-in-edge-runtime

tried using the next release. Before, I had the exact same output as @slowbaker

@bastiankistner
Copy link

I overlooked the note to use znv/compat. Replaced import and it works fine! 🙌 Can you merge it into the main branch, release it and leave a clearly visible note to use the compat import in case someone deploys to edge runtimes?

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

Successfully merging a pull request may close this issue.

5 participants