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

Supress Webpack warning for conditional require #71

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

carlonoelle
Copy link

@carlonoelle carlonoelle commented Jul 8, 2024

As stated in #67 some bundlers throw the following error/warning:

⚠ ./node_modules/tailwindcss/node_modules/picocolors/picocolors.js Critical dependency: require function is used in a way in which dependencies cannot be statically extracted

As you can see for me the error came up throuth the tailwindcss module, which i wanted to use inside a NextJS React project with nextjs@rc and react@rc which will become react 19 and nextjs 15 respectively.

NextJS uses Webpack, and Webpack for some reason has a setting called "unknownContextCritical" which checks after conditional require statements, which will then throw errors: https://github.com/webpack/docs/wiki/configuration (search the page after it)

To fix the error, simply move the require outside of the condition, its not the nicest since now the require is executed everytime, i know. But for me this is the only fix, i also created a patch-packages patch, see here (only works if you also get the error by using tailwind, otherwise you would need to patch the package yourself):

// patches/tailwindcss++picocolors+1.0.1.patch

diff --git a/node_modules/tailwindcss/node_modules/picocolors/picocolors.js b/node_modules/tailwindcss/node_modules/picocolors/picocolors.js
index 8b8a23e..0b7a529 100644
--- a/node_modules/tailwindcss/node_modules/picocolors/picocolors.js
+++ b/node_modules/tailwindcss/node_modules/picocolors/picocolors.js
@@ -1,11 +1,12 @@
 let argv = process.argv || [],
 	env = process.env
+let tty = require("tty")
 let isColorSupported =
 	!("NO_COLOR" in env || argv.includes("--no-color")) &&
 	("FORCE_COLOR" in env ||
 		argv.includes("--color") ||
 		process.platform === "win32" ||
-		(require != null && require("tty").isatty(1) && env.TERM !== "dumb") ||
+		(tty && tty.isatty(1) && env.TERM !== "dumb") ||
 		"CI" in env)
 
 let formatter =

@alexeyraspopov
Copy link
Owner

As much as I want to just merge this, using require() unconditionally going to break Edge Runtime, see #54.

How did you end up with webpack trying to analyze (bundle?) picocolors?

@carlonoelle
Copy link
Author

carlonoelle commented Jul 9, 2024

Didn't think of that. I do not have much experience handling multi runtime compatibility with a commonjs package (or any package).

My guess would be that the problem would be solved by moving the code to import/export as an ES module (at least vercel tells us that: https://vercel.com/guides/library-sdk-compatible-with-vercel-edge-runtime-and-functions#unsupported-apis), but since that crashes out the older node versions that do not support esm you would need to upgrade the min engine version.

Would have to look into how other packages handle this, maybe you can have two entry files mentioned in package.json, one for commonjs one for esm?

@alexeyraspopov
Copy link
Owner

It's getting closer and closer to the moment to consider switching to ESM package, so I'll be focusing on the strategy for 2.0, that hopefully should address these kind of problem. I wished the ecosystem would move faster. But a node package (ie users projects) are still CJS by default and shipping ESM-only package means the users must be aware of potential changes on their side. Shipping CJS+ESM package seems like a waste though and I wish it could be avoided.

@carlonoelle
Copy link
Author

Just saw this: https://github.com/component/escape-html/pull/30/files#diff-47407fecafdf5f5cd55403c3de457833ddf9b6fab45253c04e1dc4c7cb4495b1

Seems like you can port to ESM with just a simple rollbar config, maybe worth a look

@webdiscus
Copy link

webdiscus commented Jul 14, 2024

nice

@alexeyraspopov
Copy link
Owner

@webdiscus nice copypaste

How do you even blow up a simple script to this amount of code? Wow

@alexeyraspopov
Copy link
Owner

@carlonoelle I may have figured a way to suppress this warning without changing much things around. We can alias require to avoid it to be statically analyzed. If you can apply this patch to test this in your setup I'd really appreciate it:

let argv = process.argv || [],
-	env = process.env
+	env = process.env, req 
let isColorSupported =
	!("NO_COLOR" in env || argv.includes("--no-color")) &&
	("FORCE_COLOR" in env ||
		argv.includes("--color") ||
		process.platform === "win32" ||
-		(require != null && require("tty").isatty(1) && env.TERM !== "dumb") ||
+		(require != null && (req = require) && req("tty").isatty(1) && env.TERM !== "dumb") || 
		"CI" in env)

@carlonoelle
Copy link
Author

Hey @alexeyraspopov ,

sadly thats not working, i regain the errror:
image

image

I will revert to my previous patch, since for me this is a dependency of tailwind which runs at build time not in an edge runtime

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 this pull request may close these issues.

3 participants