Skip to content

Require the key be passed into createClient()#125

Merged
kyrylo merged 1 commit intoairbrake:masterfrom
Albert-IV:master
Feb 15, 2017
Merged

Require the key be passed into createClient()#125
kyrylo merged 1 commit intoairbrake:masterfrom
Albert-IV:master

Conversation

@Albert-IV
Copy link
Copy Markdown
Contributor

This change adds a check to ensure createClient() gets a key passed
into the createClient() call.

In previous versions of Airbrake the first argument was the key,
meaning that blindly upgrading the Airbrake version would result in
Airbrake throwing an error when reporting an error. This change
results in this "failing fast", giving the developer feedback on
the incorrect usage.

Fixes #124

@Albert-IV
Copy link
Copy Markdown
Contributor Author

As a side note with this change, I had to update a couple of the existing tests to include the Airbrake key during initialization. If you need me to break these into a separate commit or PR, let me know.

Also I decided to check the instance.key rather than checking the argument. If there was ever an update that pulled keys from environment variables or similar, checking the argument wouldn't be sufficient. Since we're throwing an error here, I figured the overhead of creating an Airbrake instance wouldn't be super-important.

Also if you'd like some other way of handling this than throwing an error, let me know. I think this is the only place where the library actually throws, so I'd understand if you'd like this handled differently.

lib/airbrake.js Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This check is probably not enough. We should also check projectId.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, I can take care of that. I'd seen multiple tests leave out the projectId field so I assumed that the project key wasn't 100% required (though in hindsight the key is required, so we might as well add checks for the projectId as well). Thanks for reviewing!

@kyrylo
Copy link
Copy Markdown
Contributor

kyrylo commented Feb 13, 2017

@droppedoncaprica friendly ping.

This change adds a check to ensure createClient() gets a key and
projectId passed into the createClient() call.

In previous versions of Airbrake the first argument was the key,
meaning that blindly upgrading the Airbrake version would result in
Airbrake throwing an error when reporting an error.

This change adds requirements that both the key and projectId be
supplied, "failing fast" if either of these are omitted.  This
gives the developer feedback on this quickly, rather than have
the Airbrake library throw errors when reporting errors to the
service.

Fixes #124
@kyrylo kyrylo merged commit 82aa866 into airbrake:master Feb 15, 2017
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.

2 participants