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

V7: Update config behaviour and plugin interface #759

Merged
merged 8 commits into from Mar 9, 2020
Merged

Conversation

bengourley
Copy link
Contributor

@bengourley bengourley commented Mar 5, 2020

The spec now stipulates that the only invalid configuration that should throw an error is apiKey. All others should log a warning and proceed with the default value.

The plugin interface has also been updated. init() is now load(), and the client.use() method has been disabled in favour of passing plugins in to config.plugins[].

Todo:

  • Update changelog
  • Update upgrade guide

@bugsnagbot
Copy link
Collaborator

bugsnagbot commented Mar 5, 2020

@bugsnag/browser bundle size diff

Minified Minfied + Gzipped
Before 39.03 kB 12.05 kB
After 38.96 kB 12.00 kB
± -65 bytes -49 bytes

Generated by 🚫 dangerJS against 8801042

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.

A couple of comment to consider. Mostly I'm unsure about the expect(x).toBe(x) assertions, of which there are many.


function handleXHRError () {
if (includes(getIgnoredUrls, this[REQUEST_URL_KEY])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this a bug then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, I didn't even notice. Looks like it was!

const after = process.listeners('uncaughtException').length
expect(after - before).toBe(1)
expect(c).toBe(c)
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of these assertions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Appeasing the linter :(

instantiating a client and then not referring to it leads to an unused variable error. If I remove the assignment to client, it errors with the "don't use new for side effects" rule.

I didn't want to ignore the line where it was instantiated, so this seemed like the least bad option. I'm sure there's a better way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough. We can revisit this as part of the TS conversion.

packages/plugin-react/src/index.js Show resolved Hide resolved
packages/plugin-vue/src/index.js Show resolved Hide resolved
@bengourley bengourley merged commit 12f9411 into v7 Mar 9, 2020
@bengourley bengourley deleted the v7-config-plugins branch March 9, 2020 09:53
@bengourley bengourley mentioned this pull request Apr 14, 2020
Merged
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.

None yet

3 participants