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

plugins: Support obtaining React/Vue reference after Bugsnag.start() #839

Merged
merged 11 commits into from
May 14, 2020

Conversation

bengourley
Copy link
Contributor

@bengourley bengourley commented May 6, 2020

This PR updates the browser plugins to support passing a reference to the framework in use at a point in time after Bugsnag has been started. The details are outlined below:

@bugsnag/plugin-react

Sometimes it will be the case that a React reference will not be available until after Bugsnag has been initialised.

This PR introduces an alternative API to lazily pass the reference, but maintains backward compatibility.

The two modes of operation are now as follows:

Existing

Bugsnag.start({ plugins: [new BugsnagPluginReact(React)] })
const ErrorBoundary = Bugsnag.getPlugin('react')

New (lazy)

Bugsnag.start({ plugins: [new BugsnagPluginReact()] })
const ErrorBoundary = Bugsnag.getPlugin('react')!.createErrorBoundary(React)

New (non-lazy)

Bugsnag.start({ plugins: [new BugsnagPluginReact(React)] })
const ErrorBoundary = Bugsnag.getPlugin('react')!.createErrorBoundary()

Whilst the original interface remains, the non-lazy invocation also provides a similar method for obtaining the error boundary to the lazy version. This means it is easier to type.

An attempt has been made to help prevent footguns. For example, if you instantiate the plugin without a reference to React, but then pass the result of getPlugin('react') directly to a render, the following error is displayed:

image

@bugsnag/plugin-vue

Similarly the Vue plugin has been updated to maintain backward compatibility, but also to support a framework reference later on.

Existing

Bugsnag.start({ plugins: [new BugsnagPluginVue(Vue)] })
// no use for the return type for the vue plugin

New (lazy)

Bugsnag.start({ plugins: [new BugsnagPluginVue()] })
Bugsnag.getPlugin('vue').installVueErrorHandler(Vue)

Types

Along with these updates, better typing has been provided to the plugins.

In order to achieve good types for thegetPlugin() method, rather than centralizing all of the types in the top level notifier, thereby requiring dependencies on React, Vue etc., for the top level notifiers I used the “module augmentation” method – as described in the TypeScript handbook here.

In order to do this, I needed to undo some of the changes that were made in this PR, since there we added a prepublish step to each of the top-level notifiers and plugins to bundle the types locally inside their package.

So, with each notifier and plugin having a local copy of the @bugsnag/core types inside dist/types/bugsnag-core in its package and each having a reference to the @bugsnag/core module, it means we can’t use the module augmentation technique, since it will only augment its local copy.

The reason for applying this change in the past was to prevent each notifier having an install-time dependency on @bugsnag/core, just in case the consumers were using types. We’re now in the position where the alternative is for people installing @bugsnag/js, they’d also get React, Vue, Express etc., just in case they intend to use those plugins. So now it seems like a dependency on @bugsnag/core is the better approach.

The changes I have made mean that Bugsnag modules installed together on a project will reference the same single version of @bugsnag/core, enabling each of the plugin’s definitions to augment the definition of the client’s getPlugin() method to give it a more detailed type.

In a separate PR I will add improvements to the types for plugins that were not touched here.

@bengourley bengourley requested a review from djskinner May 6, 2020 16:14
@bugsnagbot
Copy link
Collaborator

bugsnagbot commented May 6, 2020

@bugsnag/browser bundle size diff

Minified Minfied + Gzipped
Before 39.28 kB 12.12 kB
After 39.28 kB 12.12 kB
± No change No change

Generated by 🚫 dangerJS against 8d4c3d4

Copy link
Contributor

@djskinner djskinner left a comment

Choose a reason for hiding this comment

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

Implementation looks good. Maybe worth adding test coverage for the lazy loading case?

packages/plugin-react/src/index.js Show resolved Hide resolved
@bengourley bengourley force-pushed the bengourley/plugin-lazy-framework-ref branch from d75db7d to c61c04e Compare May 12, 2020 14:02
@bengourley bengourley changed the title feat(plugin-react): Defer obtaining React reference plugins: Support obtaining React/Vue reference after Bugsnag.start() May 12, 2020
@bengourley bengourley requested a review from djskinner May 12, 2020 16:32
Copy link
Contributor

@djskinner djskinner left a comment

Choose a reason for hiding this comment

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

Tested the new lazy and non-lazy APIs in a TypeScript React example project and they look good to me.

I'm not sure the backwards compatibility is working though. If I do:

Bugsnag.start({
  apiKey: 'YOUR_API_KEY',
  plugins: [new BugsnagPluginReact(React)]
})

const ErrorBoundary = Bugsnag.getPlugin('react');

Then I end up with undefined.

Consider also mentioning in changelog/upgrade guide the new preferred approach.

packages/browser/package.json Show resolved Hide resolved
@bengourley bengourley force-pushed the bengourley/plugin-lazy-framework-ref branch from ba57e21 to 7d23518 Compare May 13, 2020 15:27
@bengourley bengourley requested a review from djskinner May 13, 2020 15:27
@bengourley bengourley merged commit 5410160 into next May 14, 2020
@bengourley bengourley deleted the bengourley/plugin-lazy-framework-ref branch May 14, 2020 10:30
@bengourley bengourley mentioned this pull request May 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants