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

feat: Add scope inheritance #1343

Merged
merged 3 commits into from
Jun 1, 2018
Merged

feat: Add scope inheritance #1343

merged 3 commits into from
Jun 1, 2018

Conversation

HazAT
Copy link
Member

@HazAT HazAT commented May 31, 2018

No description provided.

@HazAT HazAT self-assigned this May 31, 2018
@HazAT HazAT requested review from jan-auer and kamilogorek May 31, 2018 19:57
@@ -89,10 +98,18 @@ export class Scope implements BaseScope {
* @param fingerprint
*/
public setFingerprint(fingerprint: string[]): void {
this.fingerprint = fingerprint;
this.fingerprint = [...fingerprint];
Copy link
Member

Choose a reason for hiding this comment

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

nit: That is not necessary.

@@ -39,9 +39,12 @@ export class Shim {
*/
public pushScope(client?: any): void {
const usedClient = client || this.getCurrentClient();
// We want to clone the last scope and not create a new one
const stack = this.getStack();
const parentScope = stack[stack.length - 1].scope;
Copy link
Member

Choose a reason for hiding this comment

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

Can we add an assertion here or even a fallback so we don't blow up in case stack is empty?

this.user = user;
this.user = {
...this.user,
...user,
Copy link
Member

Choose a reason for hiding this comment

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

This should replace instead of merge.

@HazAT HazAT merged commit f8bb04f into next Jun 1, 2018
@HazAT HazAT deleted the feature/inherit-scope branch June 1, 2018 01:04
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.

3 participants