Skip warnings in test contexts.

[jgerigmeyer]

One approach to fix #439 (comment).

[LeaVerou]

Hmm, I’m unsure.

The concept of dev vs prod goes beyond Node.js, and I would rather introduce a more abstract setting and code that tweaks it based on NODE_ENV when appropriate, rather than use NODE_ENV all over our codebase, going to the extent of mocking it in the browser! That's pretty poor DX, though I certainly appreciate the effort to use a pre-existing concept. Instructing authors to mock process.env.NODE_ENV to enable warnings could also have the unintended side effect of breaking other scripts that check for process or process.env to detect whether they’re running in Node or the browser.

How do other libraries solve this? There must be an established convention! Vue uses a separate bundle, what about others?

Btw, we should not be editing tests/ really. That's there for historical reasons, and any edits should go to the new testsuite. Otherwise we end up in the rut of maintaining two testsuites and having to keep them in sync.
FWIW we should probably even remove tests/ once all tests are ported and we are confident we haven't missed anything.

[jgerigmeyer]

The concept of dev vs prod goes beyond Node.js, and I would rather introduce a more abstract setting and code that tweaks it based on NODE_ENV when appropriate, rather than use NODE_ENV all over our codebase, going to the extent of mocking it in the browser! That's pretty poor DX, though I certainly appreciate the effort to use a pre-existing concept.

Hm, I don't think this is entirely accurate (but I'm also not tied to this approach). What I've done here is allow developers to opt out of console warnings in test environments. We're only using NODE_ENV in one place in our production codebase -- and presumably if we want this in other places for other warnings in the future, then we could abstract a single isTestEnv boolean somewhere. The only time this might impact a browser env is if a dev is running browser-based tests against Color.js and doesn't want to see warnings in the console. Authors and users will continue to see the warnings unless they have process.env.NODE_ENV = 'test' set.

Instructing authors to mock process.env.NODE_ENV to enable warnings could also have the unintended side effect of breaking other scripts that check for process or process.env to detect whether they’re running in Node or the browser.

We're not instructing anyone to mock process.env.NODE_ENV -- we're looking at whether or not that is already set to determine whether we're running in a test environment, which is a pretty common pattern.

How do other libraries solve this? There must be an established convention! Vue uses a separate bundle, what about others?

I think this is only tangentially related to dev/prod bundles, actually -- tho I agree that still might be a good idea.

Btw, we should not be editing tests/ really. That's there for historical reasons, and any edits should go to the new testsuite. Otherwise we end up in the rut of maintaining two testsuites and having to keep them in sync. FWIW we should probably even remove tests/ once all tests are ported and we are confident we haven't missed anything.

👍

[LeaVerou]

How about this?

• We add a setting for whether warnings should be shown, that modules import. That setting can check for process.env.NODE_ENV but we can easily add more conditions to it in one fell swoop, and authors can import it and override it manually.
• We add a warn() function that checks for this setting and uses console.warn() after. This not only abstracts this all away, it even allows authors to override it with a custom warn() function to redirect the warnings elsewhere. And we can do cool things with it like e.g. support a once parameter that prevents warnings from flooding the console (very very useful for depreciation warnings).

[jgerigmeyer]

How about this?

• We add a setting for whether warnings should be shown, that modules import. That setting can check for process.env.NODE_ENV but we can easily add more conditions to it in one fell swoop, and authors can import it and override it manually.
• We add a warn() function that checks for this setting and uses console.warn() after. This not only abstracts this all away, it even allows authors to override it with a custom warn() function to redirect the warnings elsewhere. And we can do cool things with it like e.g. support a once parameter that prevents warnings from flooding the console (very very useful for depreciation warnings).

I like it! And also I don't think this needs to hold up v0.5.0 😅

[lloydk]

I think abstracting the the conditional into a isTestEnv function is a good idea even if we end up using it in just one place.

Why don't we start with opting out of the warnings in the cli and not worry about the browser tests for now.

[lloydk]

How about this?

• We add a setting for whether warnings should be shown, that modules import. That setting can check for process.env.NODE_ENV but we can easily add more conditions to it in one fell swoop, and authors can import it and override it manually.
• We add a warn() function that checks for this setting and uses console.warn() after. This not only abstracts this all away, it even allows authors to override it with a custom warn() function to redirect the warnings elsewhere. And we can do cool things with it like e.g. support a once parameter that prevents warnings from flooding the console (very very useful for depreciation warnings).

I like that approach

[jgerigmeyer]

@LeaVerou I refactored this to add configurable verbose and warn options. By default it will use console.warn unless process.env.NODE_ENV === "test".

I did not set process.env.NODE_ENV=test for our local usage of htest -- would you prefer that happen here in Color.js, or should I submit a PR to htest that sets it in node-run.js for all users of htest?

Like I said earlier, I don't think this should be a blocker for v0.5.0.

[LeaVerou]

@LeaVerou I refactored this to add configurable verbose and warn options. By default it will use console.warn unless process.env.NODE_ENV === "test".

Thanks! We should use it in other places where we currently use console.warn()!

I did not set process.env.NODE_ENV=test for our local usage of htest -- would you prefer that happen here in Color.js, or should I submit a PR to htest that sets it in node-run.js for all users of htest?

Submitting it as an htest PR would be great, thank you!

Like I said earlier, I don't think this should be a blocker for v0.5.0.

Agreed.

[netlify]

✅ Deploy Preview for colorjs ready!

Name	Link
🔨 Latest commit	7eb19d9
🔍 Latest deploy log	https://app.netlify.com/sites/colorjs/deploys/65d3a3dbdcf1dd00082454af
😎 Deploy Preview	https://deploy-preview-441--colorjs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[jgerigmeyer]

Thanks! We should use it in other places where we currently use console.warn()!

@LeaVerou Can you clarify what places you're referring to? I don't see any uses of console.warn, and only one usage of console.log in the user-facing code (the rest are in website apps or the test suite). I updated the one place I found in 7eb19d9

I did not set process.env.NODE_ENV=test for our local usage of htest -- would you prefer that happen here in Color.js, or should I submit a PR to htest that sets it in node-run.js for all users of htest?

Submitting it as an htest PR would be great, thank you!

Done: htest-dev/htest#13

[jgerigmeyer]

@LeaVerou Ping on this -- are there other places you want this new warn method used?

[LeaVerou]

LGTM sorry for the delay!!