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

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

Merged
merged 4 commits into from
May 24, 2021

Conversation

mpeyper
Copy link
Member

@mpeyper mpeyper commented May 21, 2021

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.

@mpeyper mpeyper requested a review from joshuaellis May 21, 2021 03:18
Comment on lines -4 to -6
if (process.env.RHTL_DISABLE_ERROR_FILTERING) {
return () => {}
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@codecov
Copy link

codecov bot commented May 21, 2021

Codecov Report

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

Impacted file tree graph

@@            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.

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))) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Comment on lines +13 to +18
process.env = {
...process.env,
get RHTL_SKIP_AUTO_CLEANUP(): string | undefined {
throw new Error('expected')
}
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

describe('error output suppression', () => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this moved to a new file

Comment on lines -8 to +12
let cleanupCalled = false
let renderHook: ReactHooksRenderer['renderHook']
const cleanups: Record<string, boolean> = {
ssr: false,
hydrated: false
}
let renderHook: ReactHooksServerRenderer['renderHook']
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -1,4 +1,4 @@
import ReactDOM from 'react-dom'
import * as ReactDOM from 'react-dom'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@bdwain bdwain May 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Show resolved Hide resolved
Comment on lines +66 to +70
export type ServerRenderHookResult<
TProps,
TValue,
TRenderer extends ServerRenderer<TProps> = ServerRenderer<TProps>
> = RenderHookResult<TProps, TValue, TRenderer>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bdwain
Copy link

bdwain commented May 23, 2021

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
Copy link

bdwain commented May 23, 2021

this looks great! Thanks!

@mpeyper
Copy link
Member Author

mpeyper commented May 24, 2021

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.

@mpeyper
Copy link
Member Author

mpeyper commented May 24, 2021

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

Copy link
Member

@joshuaellis joshuaellis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

mpeyper commented May 24, 2021

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
Copy link
Member

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
Copy link
Member Author

mpeyper commented May 24, 2021

Ok, here we go...

@mpeyper mpeyper merged commit e4b0aa3 into main May 24, 2021
@bdwain
Copy link

bdwain commented May 24, 2021

thanks @mpeyper

@mpeyper
Copy link
Member Author

mpeyper commented May 24, 2021

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

@allcontributors
Copy link
Contributor

@mpeyper

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

@github-actions
Copy link

🎉 This PR is included in version 7.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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