feat: removed filter-console dependency and fallback if process.env is not available #624

mpeyper · 2021-05-21T03:18:53Z

What:

Remove Node specific dependencies to make running tests in a non-node environment (i.e. the browser) is easier to achieve without defining variables or polyfilling APIs.

Why:

Fixes #617

How:

Removed filter-console dependency as it was dependant on Node's util module.

Also wrapped access of process.env variables to catch if undefined and return default values.

Checklist:

Tests
Ready to be merged

There is a potentially breaking change here, depending on how you view it. Previously, the process.env.RHTL_DISABLE_ERROR_FILTERING check was performed inside the suppressErrorOutput function, which was fine when it was first introduced.

Later, suppressErrorOutput was exposed out of the API so that consumers using the pure imports could opt into the suppression as well, however, setting the RHTL_DISABLE_ERROR_FILTERING variable had the possibility of making explicit calls to suppressErrorOutput a no-op.

I view this as a bug introduced when exposing suppressErrorOutput, rather than a breaking change being introduced here now, and I doubt anyone is relying on this odd combination of behaviour, but if others feel strongly I'm happy to either retain the existing behaviour or make this a major bump. Feedback welcome.

Finally, I've snuck in a few pieces of cleanup from dropping Node 10 support as well as some small tweaks that makes the final output of the code not rely on @babel/runtime at all. I'm loathed to remove @babel/runtime dependency all together as I feel it's too easy for a change to introduce the need for it again, which is relatively unknown until you publish a broken version.

…s not available Fixes #617

mpeyper · 2021-05-21T03:20:35Z

src/core/console.ts

-  if (process.env.RHTL_DISABLE_ERROR_FILTERING) {
-    return () => {}
-  }


This is the breaking change I referred to in the PR description

mpeyper · 2021-05-21T03:21:28Z

src/core/console.ts

@@ -1,24 +1,40 @@
-import filterConsole from 'filter-console'
+const consoleFilters = [
+  /^The above error occurred in the <.*?> component:/, // error boundary output


I've slightly loosened what this will match, in case a Wrapper throws instead of the TestComponent

codecov · 2021-05-21T03:23:06Z

Codecov Report

Merging #624 (eb77f48) into main (e11b63a) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #624   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           15        15           
  Lines          218       235   +17     
  Branches        29        33    +4     
=========================================
+ Hits           218       235   +17

Impacted Files	Coverage Δ
src/dom/pure.ts	`100.00% <ø> (ø)`
src/pure.ts	`100.00% <ø> (ø)`
src/server/pure.ts	`100.00% <ø> (ø)`
src/core/cleanup.ts	`100.00% <100.00%> (ø)`
src/core/console.ts	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e11b63a...eb77f48. Read the comment docs.

mpeyper · 2021-05-21T03:23:42Z

src/core/console.ts

-      methods: ['error']
+  const error = (...args: Parameters<typeof originalError>) => {
+    const message = typeof args[0] === 'string' ? args[0] : null
+    if (!message || !consoleFilters.some((filter) => filter.test(message))) {


Just to call it out, filter-console would format the string before testing it against the patter (that's why it had a dependency on util), but I found this wasn't necessary for the cases we are trying to filter. Not doing this may be an issue if how the calls from upstream are made ever change or if we introduce any new cases in the future (although I don't think that is likely).

mpeyper · 2021-05-21T03:24:41Z

src/dom/__tests__/autoCleanup.noProcessEnv.test.ts

+    process.env = {
+      ...process.env,
+      get RHTL_SKIP_AUTO_CLEANUP(): string | undefined {
+        throw new Error('expected')
+      }
+    }


I couldn't remove process.env from the test environment all together (other things started to crash), but I could make it error when trying to access the variables.

mpeyper · 2021-05-21T03:25:38Z

src/dom/__tests__/errorHook.test.ts

@@ -142,51 +142,4 @@ describe('error hook tests', () => {
      expect(result.error).toBe(undefined)
    })
  })
-
-  describe('error output suppression', () => {


this moved to a new file

mpeyper · 2021-05-21T03:27:38Z

src/server/__tests__/autoCleanup.disabled.test.ts

-  let cleanupCalled = false
-  let renderHook: ReactHooksRenderer['renderHook']
+  const cleanups: Record<string, boolean> = {
+    ssr: false,
+    hydrated: false
+  }
+  let renderHook: ReactHooksServerRenderer['renderHook']


I've improved quite a few of the server renderer tests to actually test the behaviour pre/post hydration. Some already did this but others not so I made them consistent.

mpeyper · 2021-05-21T03:28:21Z

src/dom/pure.ts

@@ -1,4 +1,4 @@
-import ReactDOM from 'react-dom'
+import * as ReactDOM from 'react-dom'


This prevents the import relying on @babel/runtime for default import interop.

so this works the same either way? interesting. i would expect you to need to add .default where you used ReactDOM before

It's interesting because import ReactDOM and import * as ReactDOM are not equivalent as per the ES6 import spec, but most compilers/bundlers treat them interchangeably. Babel does this by inserting the default import interop helper when there is no default export for the module, like in React/ReactDOM's case. By changing the the import * as ..., it's actually more correct for these libraries.

src/pure.ts

mpeyper · 2021-05-21T03:30:59Z

src/types/index.ts

+export type ServerRenderHookResult<
+  TProps,
+  TValue,
+  TRenderer extends ServerRenderer<TProps> = ServerRenderer<TProps>
+> = RenderHookResult<TProps, TValue, TRenderer>


I introduced a few server render specific types here to help out in the tests. The technically aren't used by the production code, but they don't hurt it either.

src/server/pure.ts

src/core/console.ts

src/core/cleanup.ts

bdwain · 2021-05-23T01:27:42Z

there are uses of process.env in dont-cleanup-after-each.js and disable-error-filtering.js too. Maybe wrap those in a try/catch that throws a helpful error or something?

bdwain · 2021-05-23T01:28:19Z

this looks great! Thanks!

mpeyper · 2021-05-24T02:37:32Z

there are uses of process.env in dont-cleanup-after-each.js and disable-error-filtering.js too. Maybe wrap those in a try/catch that throws a helpful error or something?

Good point. They might be used in setup files and get bundled in too, so we should protect them in a similar same way.

… try/catch is being used

mpeyper · 2021-05-24T03:40:16Z

I'm getting cold feet about not pushing it as a breaking change. @joshuaellis what are your thoughts?

joshuaellis

I think this is fair to assume it's a fix, if I understand correctly, it's all about process.env not breaking in the browser but using that env variable will still stop the call, so in theory everyone's tests should run the same.

If someone's done a hack // workaround to solve this themselves then there's a cause it'll break, but you can hardly account for that.

mpeyper · 2021-05-24T07:57:53Z

Not quite @joshuaellis. Currently, if someone set RHTL_DISABLE_ERROR_FILTERING to true and then imported and called suppressErrorOutput it would skip the suppression and the error logs would be produced.

With this change, explicit calls to suppressErrorOutput would override the env variable and suppress the logs.

joshuaellis · 2021-05-24T08:03:51Z

Aha, thank you for clearing that up. Then it still sounds like it could be a breaking change, albeit not exactly dramatic, but given previous history we've had with "breaking people's test suites" I think we should play it safe and release as breaking.

mpeyper · 2021-05-24T22:46:12Z

Ok, here we go...

bdwain · 2021-05-24T22:50:03Z

thanks @mpeyper

mpeyper · 2021-05-24T22:51:51Z

@all-contributors please add @bdwain for bug, review 😄

allcontributors · 2021-05-24T22:51:59Z

@mpeyper

I've put up a pull request to add @bdwain! 🎉

github-actions · 2021-05-24T22:55:19Z

🎉 This PR is included in version 7.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

feat: removed filter-console dependency and fallback if process.env i…

a16578a

…s not available Fixes #617

mpeyper requested a review from joshuaellis May 21, 2021 03:18

mpeyper commented May 21, 2021

View reviewed changes

src/pure.ts Show resolved Hide resolved

mpeyper commented May 21, 2021

View reviewed changes

bdwain reviewed May 23, 2021

View reviewed changes

src/server/pure.ts Show resolved Hide resolved

bdwain reviewed May 23, 2021

View reviewed changes

src/core/console.ts Show resolved Hide resolved

bdwain reviewed May 23, 2021

View reviewed changes

src/core/cleanup.ts Show resolved Hide resolved

bdwain approved these changes May 23, 2021

View reviewed changes

fix: protect import helpers for setting env variables and comment why…

e41c156

… try/catch is being used

Merge branch 'main' into better-browser-support

5657d97

joshuaellis approved these changes May 24, 2021

View reviewed changes

Merge branch 'main' into better-browser-support

eb77f48

mpeyper merged commit e4b0aa3 into main May 24, 2021

allcontributors bot mentioned this pull request May 24, 2021

docs: add bdwain as a contributor #628

Merged

github-actions bot added the released label May 24, 2021

mpeyper deleted the better-browser-support branch May 25, 2021 03:03

ohardy mentioned this pull request May 25, 2021

Cannot find module 'filter-console' from 'node_modules/@testing-library/react-hooks/lib/core/console.js' #629

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: removed filter-console dependency and fallback if process.env is not available #624

feat: removed filter-console dependency and fallback if process.env is not available #624

mpeyper commented May 21, 2021

mpeyper May 21, 2021

mpeyper May 21, 2021

codecov bot commented May 21, 2021 •

edited

mpeyper May 21, 2021

mpeyper May 21, 2021

mpeyper May 21, 2021

mpeyper May 21, 2021

mpeyper May 21, 2021

bdwain May 23, 2021 •

edited

mpeyper May 24, 2021

mpeyper May 21, 2021

bdwain commented May 23, 2021 •

edited

bdwain commented May 23, 2021

mpeyper commented May 24, 2021

mpeyper commented May 24, 2021

joshuaellis left a comment

mpeyper commented May 24, 2021 •

edited

joshuaellis commented May 24, 2021

mpeyper commented May 24, 2021

bdwain commented May 24, 2021

mpeyper commented May 24, 2021

allcontributors bot commented May 24, 2021

github-actions bot commented May 24, 2021

		@@ -1,4 +1,4 @@
		import ReactDOM from 'react-dom'
		import * as ReactDOM from 'react-dom'

feat: removed filter-console dependency and fallback if process.env is not available #624

feat: removed filter-console dependency and fallback if process.env is not available #624

Conversation

mpeyper commented May 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 21, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdwain May 23, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdwain commented May 23, 2021 • edited

bdwain commented May 23, 2021

mpeyper commented May 24, 2021

mpeyper commented May 24, 2021

joshuaellis left a comment

Choose a reason for hiding this comment

mpeyper commented May 24, 2021 • edited

joshuaellis commented May 24, 2021

mpeyper commented May 24, 2021

bdwain commented May 24, 2021

mpeyper commented May 24, 2021

allcontributors bot commented May 24, 2021

github-actions bot commented May 24, 2021

codecov bot commented May 21, 2021 •

edited

bdwain May 23, 2021 •

edited

bdwain commented May 23, 2021 •

edited

mpeyper commented May 24, 2021 •

edited