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

fix(react): import correctly @honeybadger-io/js #1024

Merged
merged 1 commit into from
Feb 17, 2023

Conversation

subzero10
Copy link
Member

Fixes: #1020

Status

READY

Description

This was a brain teaser 😓 .
This PR fixes a bug I introduced with the release of the Collect User Feedback feature. The bug was introduced because I couldn't run the tests 🤦 .

Some context:

  • With the "Collect User Feedback", I added a new property in the react component to show the form automatically in case of an error (showUserFeedbackForm)
  • I created a test for it, but it was failing with HTTP 403, even though I tried mocking the fetch api. I realised that it was importing the server implementation of Honeybadger (as it is the main entry point of the @honeybadger-io/js package.
  • So, since we always need the client side implementation, I changed the import to be explicit and imported @honeybadger-io/js/dist/browser.
  • Hooray! My tests passed!

This is what I missed:

  • Some frameworks use SSR, and in this case importing the browser implementation will create errors; and this is what happens with the next.js app.

The fix:

  • Always import @honeybadger-io/js and let bundlers decide which entry point to use depending on the environment. I tested this in @BethanyBerkowitz's repo by adding a line in _app.js:
// honeybadger.errorHandler should exist only on server implementation
console.log(typeof window === 'undefined' ? 'server' : 'browser', honeybadger.errorHandler) 

When running npm run dev, I get this on the terminal:

server [Function: bound errorHandler]

and this on the browser console:

browser undefined

Which indicates that everything is imported correctly.

  • Fixing the tests: jest supports a resolver config which can be used for these cases (to make it respect the browser field in package.json). However, it's not supported in react-scripts so I explicitly import the browser implementation.

@subzero10 subzero10 added bug react @honeybadger-io/react labels Feb 17, 2023
@subzero10 subzero10 self-assigned this Feb 17, 2023
Copy link
Contributor

@BethanyBerkowitz BethanyBerkowitz left a comment

Choose a reason for hiding this comment

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

This was a tricky one! Glad you were able to find it.

@subzero10 subzero10 merged commit 10b4a5f into master Feb 17, 2023
@subzero10 subzero10 deleted the fix_import_universal_js_package_#1020 branch February 17, 2023 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug react @honeybadger-io/react
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Version 5.1.1 is broken on Next.js
2 participants