feat(core): Set default scope for BaseClient methods

[timfish]

client.captureException/captureMessage/captureEvent do not include current scope in events by default. This is in contrast to captureException/captureMessage/captureEvent exported from every SDK which include the current scope by default.

This PR changes the parameter names (scope > currentScope) and adds jsdocs to make this more clear. It also changes the parameters to use the scope interface instead of the scope class.

[AbhiPrasad]

This feels right to me - thoughts but I want to make sure @mydea is fine with it.

[mydea]

It feels OK, but I guess the main downside is that it can be confusing when working with multi clients 🤔 You'll easily get leakage in this scenario:

const client = new BrowserClient();
const client2 = new BrowserClient();

setCurrentClient(client);
client2.captureException(); // <-- will have scope from `client` applied

Not sure how much we care about this, though...

Generally, what's the use case to call this directly on the client?

[timfish]

Generally, what's the use case to call this directly on the client?

it can be confusing when working with multi clients

Working with multiple clients is probably the only use case. The client is passed to integrations though so users might use it there if they don't know it's limitations. Some jsdocs that say you probably want to be calling X instead might help.

It just felt like a bit of a footgun that client.captureX doesn't include current scope but includes both isolation and global scopes implicitly. I spent a good chunk of time trying to work out why it wasn't working and I know the codebase more than most users will. Maybe if we rename the scope parameters to currentScope it would make it more obvious that what the parameter is used for?

[mydea]

I think my main concern with this is that it does not play well with this scenario:

const scope1 = new Scope();
const scope2 = new Scope();
const client1 = new Client();
const client2 = new Client();

scope1.setClient(client1);
scope2.setClient(client2);

client1.captureException(...); // <-- gets some other scope applied, not scope1

obviously this is also not great today, but I'd argue that it's better to not apply any scope here than to apply the incorrect one...?

Overall I'd tend to improve this through documentation/jsdoc. E.g. it makes sense to add to the client.captureXXX methods something like "You have to pass a scope, the current scope is not automatically applied" or something along these lines? 🤔

[timfish]

I'd argue that it's better to not apply any scope here than to apply the incorrect one

Ah yes, agreed!

Overall I'd tend to improve this through documentation/jsdoc

Ok, I'll change this PR to do that.

[timfish]

Ah sorry, one more thought...

You have to pass a scope, the current scope is not automatically applied

If you have to pass a scope, should we make the scope parameter non-optional? Is there a case where you wouldn't want to pass the current scope to these client methods?

Unfortunately, we'd need to change the order of the parameters to do this...

[AbhiPrasad]

If you have to pass a scope, should we make the scope parameter non-optional? Is there a case where you wouldn't want to pass the current scope to these client methods?

I think this is too much friction for now, but we should evaluate this for later on