Uh oh!
There was an error while loading. Please reload this page.
ref(core): Replace Scope.clone() with non-static scope.clone() - #9801
Conversation
size-limit report 📦
|
timfish
commented
Dec 13, 2023
We used Not sure what the best solution is. |
mydea
commented
Dec 13, 2023
So for now functioncloneScope(scope?: Scope): Scope{returnscope ? scope.clone() : newScope();}Or what do you mean? |
The The previous clone copied all the properties off the supplied scope: publicstaticclone(scope?: Scope): Scope{constnewScope=newScope();if(scope){newScope._breadcrumbs=[...scope._breadcrumbs];newScope._tags={ ...scope._tags};newScope._extra={ ...scope._extra};newScope._contexts={ ...scope._contexts}; |
timfish
commented
Dec 13, 2023
We can fudge this with types in the Electron SDK but my concern would be that if any private properties in |
mydea
commented
Dec 14, 2023
Ahh, I see, so you're basically duck-typing IMHO maybe the better solution for this is to have something like |
timfish
commented
Dec 14, 2023
Yep!
That would work. Want me to do the PR? |
mydea
commented
Dec 14, 2023
I think I'd hold off on that until we have the scope changes done, as this is a bit in flux right now! This should happen very soon though, I'll ping you then! :) |
In #9801, we introduced a regression that if you pass a function as `captureContext` to capture methods, the returned scope is not used. The cause for this was a confusion on my end, based on the slightly weird way this works in `scope.update(fn)` - we don't actually merge this or update the scope based on the return value of `fn`, but `fn` receives the `scope` as argument, does nothing with the return type of `fn` and just returns it - which we didn't use, because I assumed that `scope.update` would actually return the scope (also, the return type of it is `this` which is not correct there). This PR changes this so that the returned scope of `fn` is actually merged with the scope, same as if you'd pass a `scope` directly - so this is fundamentally the same now: ```js const otherScope = new Scope(); scope.update(otherScope); scope.update(() => otherScope); ``` (which before would have had vastly different outcomes!) I added a bunch of tests to verify how this works/should work. Fixes#10686
In #9801, we introduced a regression that if you pass a function as `captureContext` to capture methods, the returned scope is not used. The cause for this was a confusion on my end, based on the slightly weird way this works in `scope.update(fn)` - we don't actually merge this or update the scope based on the return value of `fn`, but `fn` receives the `scope` as argument, does nothing with the return type of `fn` and just returns it - which we didn't use, because I assumed that `scope.update` would actually return the scope (also, the return type of it is `this` which is not correct there). This PR changes this so that the returned scope of `fn` is actually merged with the scope, same as if you'd pass a `scope` directly - so this is fundamentally the same now: ```js const otherScope = new Scope(); scope.update(otherScope); scope.update(() => otherScope); ``` (which before would have had vastly different outcomes!) I added a bunch of tests to verify how this works/should work. Fixes#10686
…#10737) Backport to v7. In #9801, we introduced a regression that if you pass a function as `captureContext` to capture methods, the returned scope is not used. The cause for this was a confusion on my end, based on the slightly weird way this works in `scope.update(fn)` - we don't actually merge this or update the scope based on the return value of `fn`, but `fn` receives the `scope` as argument, does nothing with the return type of `fn` and just returns it - which we didn't use, because I assumed that `scope.update` would actually return the scope (also, the return type of it is `this` which is not correct there). This PR changes this so that the returned scope of `fn` is actually merged with the scope, same as if you'd pass a `scope` directly - so this is fundamentally the same now: ```js const otherScope = new Scope(); scope.update(otherScope); scope.update(() => otherScope); ``` (which before would have had vastly different outcomes!) I added a bunch of tests to verify how this works/should work. Fixes#10686
To avoid this static method there. It is deprecated to use
Scope.clone()(but still works), but better to just usescope.clone()ornew Scope()directly.