V7: Simplify client configuration and store config privately #656
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bengourley
commented
Nov 29, 2019
| let warningMessage = '' | ||
|
|
||
| if (opts.endpoints && opts.endpoints.notify && !opts.endpoints.sessions) { | ||
| warningMessage += 'notify endpoint is set but sessions endpoint is not. No sessions will be sent.' |
There was a problem hiding this comment.
it's now impossible to get into this state, as of #647
bengourley
force-pushed
the
v7-client-config-simplification
branch
3 times, most recently
from
69d8c07
to
31c7527
Compare
djskinner
approved these changes
Nov 29, 2019
| @@ -53,7 +46,7 @@ describe('plugin: window onerror', () => { | |||
| // const client = new Client(VALID_NOTIFIER) | |||
| // const payloads = [] | |||
| // client.setOptions({ apiKey: 'API_KEY_YEAH' }) | |||
| // client.configure() | |||
| // client._configure() | |||
There was a problem hiding this comment.
It's only a comment but I think this should be unchanged. Maybe the all commented code needs updating or just removing? Same for other places in this file.
There was a problem hiding this comment.
We have a backlog ticket to convert all these commented out unit tests into maze runner tests.
I'll undo the change.
| @@ -64,7 +59,7 @@ describe('plugin: unhandled rejection', () => { | |||
| // it('works with DOMExceptions', done => { | |||
| // const client = new Client(VALID_NOTIFIER) | |||
| // setOptions({ apiKey: 'API_KEY_YEAH' }) | |||
| // client.configure() | |||
| // client._configure() | |||
There was a problem hiding this comment.
Commented code as per other comment
bengourley
force-pushed
the
v7-client-config-simplification
branch
from
31c7527
to
8c2eb11
Compare
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The existing configuration routine was laborious and can be simplified to be done in one fell swoop
This is just an implementation detail really – this logic is done internally in each exported notififer – but it removes some methods from the public client interface (making it compliant with the spec).
var NOTIFIER_DETAILS = { name: 'Bugsnag x', url: 'https://bugsnag.com', version: '0.0' } - const client = new Client(NOTIFIER_DETAILS) - client.setOptions(userOpts) - client.configure(customSchema) + const client = new Client(userOpts, schema, NOTIFIER_DETAILS)One benefit of the configuration being done via the constructor is that we can remove some of the "client not configured" errors – it's now impossible to have an instance of a client that's not configured.
Since loads of unit tests create
Clients directly, this changeset is pretty big, but it does end up removing a lot of boilerplate lines from those tests.The other change in this PR is to store config at
client._config(_to signify that it is private) and remove itspublicdefinition from the types to match up with the spec.