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

use continuation-local-storage to store session info #3

Merged
merged 5 commits into from
Jan 30, 2018

Conversation

Eskibear
Copy link
Owner

@Eskibear Eskibear commented Jan 18, 2018

About implementation of continuation-local-storage, refer to:
nodejs/node-v0.x-archive#5243

Now simply use:

TelemetryWrapper.registerCommand("commandName",
        (args: any[]): void => {
            // TODO
            TelemetryWrapper.currentSession().id;
        }
);

@Eskibear Eskibear self-assigned this Jan 18, 2018
@Eskibear Eskibear requested a review from jdneo January 18, 2018 02:46
@Eskibear
Copy link
Owner Author

@akaroml

@Eskibear
Copy link
Owner Author

@ansyral

@@ -60,6 +69,10 @@ export module TelemetryWrapper {
}
}

export function currentSession() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Currently, the session and reporter are in one class. They have different concerns and roles so I would suggest that we separate them.

Copy link
Owner Author

Choose a reason for hiding this comment

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

@akaroml If I got it right, they're not defined in one class.
Session is defined in Session.ts
TelemetryReporter is defined in package vscode-extension-telemetry.

TelemetryWrapper.ts defines static variables and functions, e.g. a static TelemetryReporter instance, and a static method currentSession() .

Copy link
Owner Author

Choose a reason for hiding this comment

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

got that, Session and TelemetryReporter decoupled.

@Eskibear Eskibear merged commit 5e655c6 into master Jan 30, 2018
@Eskibear Eskibear deleted the localstorage branch August 29, 2018 02:32
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