use of process.env and filter-console library prevents running tests in browser environment

[bdwain]

• react-hooks-testing-library version: 5.1.2
• react version: 17
• react-dom version (if applicable): 17
• react-test-renderer version (if applicable): n/a
• node version: 14
• npm (or yarn) version: yarn 2

Relevant code or config:

import '@testing-library/react-hooks/dom/pure';

What you did:

just imported the library

What happened:

ERROR in ./node_modules/filter-console/index.js 2:13-28
Module not found: Error: Can't resolve 'util' in '/Users/bwain/dev/react-hooks-testing-lib-process-bug/node_modules/filter-console'

BREAKING CHANGE: webpack < 5 used to include polyfills for node.js core modules by default.
This is no longer the case. Verify if you need this module and configure a polyfill for it.

I've also seen other errors (depending on my exact config) due to use of process.env for things like process.env.RHTL_SKIP_AUTO_CLEANUP.

Reproduction:

https://github.com/bdwain/react-hooks-testing-lib-process-bug

Problem description:

react testing library can not run in a browser environment because it uses process.env and because it pulls in the filter-console library, which is targeted at node and uses the util library under the hood.

Webpack used to provide a node polyfill for you automatically in webpack <5, which likely masked the issue for a long time. Now that they've stopped polyfilling node, issues like this are being exposed.

While Jest is probably the most widely use test runner right now (and it uses a node environment with JSDom), many people prefer to run their tests in an actual browser using tools like karma. There's no good way to do that though with the current usages of process.env and filter-console. If we were to start modifying our test environments to polyfill node modules or stub them out somehow, we'd lose our ability to detect usages of those node modules in tests, which decreases their value.

Suggested solution:

If the usages of node modules could be replaced with things that work in both a browser environment and a node environment, that would open up usage to a lot of people.

Alternatively, the things that use node modules could be made to be separately imported from the rest of the library, so that they are opt-in rather than forced on you.

[mpeyper]

Thanks @bdwain for raising this. When I first read it I was sure you must not be importing from the .../pure module as the whole point for it existing is to skip the cleanup registration and console suppression, which is true, but when double checking my thoughts I noticed this in the code. If you follow the import chain from there, suppressErrorOutput end up importing filter-console and triggering the reported issue.

While importing filter-console is not necessarily an issue (it doesn't implicitly filter, just imports it so the user can set it up themselves), I agree that we should try to avoid node-specific dependencies wherever possible.

I wanted to avoid writing the console filtering ourselves as it would be just another thing we need to maintain, but unless there is a reasonable alternative, I cannot think of a better solution.

[bdwain]

thanks! Let me know if there's anything I can do to help make this happen.

[mpeyper]

Let me know if there's anything I can do to help make this happen.

PRs are always welcome 😉

Basically, this is the code we need to replace. We want to prevent calls to console.error with those patterns from actually printing to the console (the reasoning for which can be found here).

[bdwain]

cool I can check that out. what about the configuration via process.env. Any thoughts on the best way to replace that? I know webpack can use the DefinePlugin to replace it at build time, but ideally the consumer of the library does not need to know about those env variables in order for them to work properly in the default case.

Are they technically a supported API? Because if they are, removing them would be a breaking change, which is not ideal.

[mpeyper]

The env variables are a supported and documented part of the API, and is consistent with the auto cleanup approach we use, as well as react-testing-library. I could be convinced that it should be moved into enableErrorOutputSuppression so the pure module does not hit that line, assuming webpack is fine with it being there as long as that line is not run. Either that or conditionally testing perhaps something like process?.env?.RHTL_DISABLE_ERROR_FILTERING ?? false, if that works in this case?

[mpeyper]

Sorry, hit the wrong button

[bdwain]

i think the process.env stuff already works when you use pure. It just seems odd to say that you need to be running in a node environment to get the impure functionality, when the two aren't really related except that the impure functionality requires node.

I tried process?.env?.RHTL_DISABLE_ERROR_FILTERING ?? false but it errored because process was not defined. What about this though? You could wrap usages of process.env in a try/catch, and then if errors get caught, use the default behavior. That would allow people to get the defaults even if they are running in a browser environment, and if they want to use the env things, they can opt in however they would normally do that (e.g. webpack define plugin or an equivalent for other tools).

example

//switch
  if (typeof afterEach === 'function' && !process.env.RHTL_SKIP_AUTO_CLEANUP) {
    afterEach(async () => {
      await cleanup()
    })
  }
//to
  let skipAutoCleanup;
  try {
     skipAutoCleanup = process.env.RHTL_SKIP_AUTO_CLEANUP;
  }
  catch(err){
     skipAutoCleanup = false;
  }
  if (typeof afterEach === 'function' && !skipAutoCleanup) {
    afterEach(async () => {
      await cleanup()
    })
  }

for a second i was thinking you could make a helper to read env variables so it wouldn't have to be so verbose each time, but that would break a lot of tools that populate env variables by looking for the actual string in the code and replacing it inline (like webpack define plugin)

[mpeyper]

You can probably use typeof process !== "undefined" && typeof process.env !== "undefined" instead, which you could pull out into a const hasProcessEnv = ... and then const skipAutoCleanup = hasProcessEnv && process.env.RHTL_SKIP_AUTO_CLEANUP?

Edit: actually, just thinking that through, I think it gets problematic if somone does replace the variable inline and then run in a non-node environment (hasProcessEnv would be false so skipAutoCleanup would also be false, even if process.env.RHTL_SKIP_AUTO_CLEANUP was replaced with true). I think your suggestion is fine. I'd be tempted to pull it out into a function though:

function skipAutoCleanup() {
  try {
     return process.env.RHTL_SKIP_AUTO_CLEANUP;
  }
  catch(err){
     return false;
  }
}

// ...

  if (typeof afterEach === 'function' && !skipAutoCleanup()) {
    afterEach(async () => {
      await cleanup()
    })
  }

[bdwain]

that might not work because of tools that find/replace process.env. They won't replace usages of process or process.env on their own

for a second i was thinking you could make a helper to read env variables so it wouldn't have to be so verbose each time, but that would break a lot of tools that populate env variables by looking for the actual string in the code and replacing it inline (like webpack define plugin)

[mpeyper]

Yeah, I realised that in my edit. I think you're on the right track with it.

[bdwain]

cool ill messing with a PR

[mpeyper]

Hey @bdwain, how are you going with this?

[bdwain]

I haven't been able to start yet. I should be able to get to it this weekend though.

[mpeyper]

I had a quick play around with it last night and I have something that is working. I can clean it up and make a PR for you to review or I can hold off and wait to see what you come up with this weekend (I don't want to steal your opportunity to contribute, I was looking at something else and removing the filter-console dependency was helpful for that as well). Whatever you prefer?

[bdwain]

If you have something working then go for it! I’m just happy to get the node errors fixed. I can review the pr.

…

On Thu, May 20, 2021 at 5:51 PM Michael Peyper ***@***.***> wrote: I had a quick play around with it last night and I have something that is working. I can clean it up and make a PR for you to review or I can hold off and wait to see what you come up with this weekend (I don't want to steal your opportunity to contribute, I was looking at something else and removing the filter-console dependency was helpful for that as well). Whatever you prefer? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#617 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA6MGDUR4XOOJ5Q537N7F5DTOWG7LANCNFSM44UQSP7Q> .

[mpeyper]

@bdwain, GH is not letting be add you as a reviewer, but you can find the PR in the above link.

[github-actions]

🎉 This issue has been resolved in version 7.0.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀