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: logger wrapper package #163

Merged
merged 7 commits into from
Jan 31, 2021
Merged

feat: logger wrapper package #163

merged 7 commits into from
Jan 31, 2021

Conversation

bd82
Copy link
Member

@bd82 bd82 commented Jan 14, 2021

No description provided.

@bd82
Copy link
Member Author

bd82 commented Jan 14, 2021

Need one more test to check the branch where the configuration was not "affected".

@bd82
Copy link
Member Author

bd82 commented Jan 14, 2021

and need an example that asserts the logging props exist on the package.json of the extension.

@bd82
Copy link
Member Author

bd82 commented Jan 17, 2021

READY FOR REVIEW

Copy link

@tal-sapan tal-sapan left a comment

Choose a reason for hiding this comment

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

Please consider the comments

"debug",
"trace"
],
"default": "info",

This comment was marked as resolved.

Copy link
Member Author

Choose a reason for hiding this comment

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

oops


function initLogger(context: ExtensionContext): void {
const meta = JSON.parse(
readFileSync(resolve(context.extensionPath, "package.json"), "utf8")

This comment was marked as resolved.

Copy link
Member Author

Choose a reason for hiding this comment

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

Normally I'd agree.

However I think this is an exception to the rule.

  1. This function is called directly from activate
  2. activate will need to finish (be resolved) before the extension is allowed to start.
  3. reading a small file from the local file-system should be fast even if it is blocking, it is not a network call.

So I am not sure this example scenario warrants adding the complexity of async code.

Choose a reason for hiding this comment

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

activate can return be async and return a promise so there isn't much complexity (just add async on the function and call await readFile(...) instead of readFileSync(...)). There are some sample extensions that do it.
You could get a warning in vscode (in the console or maybe some output channel - not sure, I've only seen it during tests) about high CPU usage if you use sync file system access. It will also block other extensions from being loaded during this time, see:
https://github.com/microsoft/vscode/wiki/Explain-extension-causes-high-cpu-load

Copy link
Member Author

Choose a reason for hiding this comment

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

changed to async due to concerns with blocking the extensionHost process

Comment on lines 17 to 18
// By asserting the existence of the properties in the package.json
// at runtime, we avoid many copy-pasta mistakes...

Choose a reason for hiding this comment

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

I think in productive extensions this should be done in a unit test... I don't know if we should encourage assertions like this in productive code

Copy link
Member Author

Choose a reason for hiding this comment

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

performing these assertions inside a unit test may be more optimal / cleaner.
However, these strings are one of the most likely places for user errors.

So I think the pros outweigh the cons.

Also consider. runtime checks and assertions are not conceptually wrong:

And it may even be possible to remove them during compile/bundling in a more "productive" example.

Choose a reason for hiding this comment

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

If you remove the assertion at runtime then ok... could you add it to the example?
I couldn't understand how to make unassert work from their documentation, can it be used only for the packaged vsix so the assertions will still run when you test the extension locally?
Also, I'm not sure from the documentation that it will work with a deconstructed assert. In their example they call assert.ok and since they manipulate the AST they might not support the deconstructed version.

Choose a reason for hiding this comment

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

It looks like it has a webpack loader so maybe this is what should be used:
https://github.com/unassert-js/webpack-unassert-loader

Copy link
Member Author

Choose a reason for hiding this comment

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

could you add it to the example?

The example needs to stay simple (generic OSS) and does not currently include any bundling where the webpack-unassert-loader can be added.

I think the assertions add more value to the example than possible issues (even if they remain in productive code).
Because it is likely a consumer of this library would copy/paste the example and may forget to change the constant values.

And I'd rather not add bundling / testing to these examples as that would add other opinionated solutions / tools
beyond just demonstrating how to use the vscode-logging libraries.

A cleaner solution using tests is available here:

Copy link
Member Author

Choose a reason for hiding this comment

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

My points above were meant to show it is not fundamentally wrong to use runtime assertions
and that those assertions could even be removed from "productive" code, so those more advanced solutions
are very possible to be implemented by end users in their own productive VSCode extensions.

Choose a reason for hiding this comment

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

Okay

Comment on lines 13 to 36
const meta = JSON.parse(
readFileSync(resolve(context.extensionPath, "package.json"), "utf8")
);

// By asserting the existence of the properties in the package.json
// at runtime, we avoid many copy-pasta mistakes...
const configProps = meta?.contributes?.configuration?.properties;
ok(configProps?.[LOGGING_LEVEL_PROP]);
ok(configProps?.[SOURCE_LOCATION_PROP]);

const extLogger = configureLogger({
extName: meta.displayName,
logPath: context.logPath,
logOutputChannel: window.createOutputChannel(meta.displayName),
// set to `true` if you also want your VSCode extension to log to the console.
logConsole: false,
loggingLevelProp: LOGGING_LEVEL_PROP,
sourceLocationProp: SOURCE_LOCATION_PROP,
subscriptions: context.subscriptions,
onDidChangeConfiguration: workspace.onDidChangeConfiguration,
getConfiguration: workspace.getConfiguration
});

setLogger(extLogger);

This comment was marked as resolved.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes good idea 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks good in an empty extension but won't in a true productive extension.

@bd82
Copy link
Member Author

bd82 commented Jan 17, 2021

@tal-sapan I could demonstrate a more "productive" (and opinionated) integration example of the logger wrapper
here: in our ECMAScript mono-repo template.

That repo is targeted more towards internal consumption, and I feel more comfortable being more opinionated there.

WDYT?

@bd82
Copy link
Member Author

bd82 commented Jan 17, 2021

Copy link

@tal-sapan tal-sapan left a comment

Choose a reason for hiding this comment

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

@tal-sapan have a look at : SAP-samples/ecmascript-monorepo-template#134

I commented there. I think it's a good idea to add it to that example.

export function activate(context: ExtensionContext): void {
initLogger(context);
getLogger().info("extension is active, hip hip hurray!");
registerCommands(context.subscriptions);

This comment was marked as resolved.

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks

@bd82
Copy link
Member Author

bd82 commented Jan 18, 2021

@tal-sapan last commit should include fixes for everything except the assertions which I would rather keep as is.

@bd82
Copy link
Member Author

bd82 commented Jan 18, 2021

Will manually test it again now

@bd82
Copy link
Member Author

bd82 commented Jan 18, 2021

Will manually test it again now

Manual test successful

@tal-sapan
Copy link

@tal-sapan last commit should include fixes for everything except the assertions which I would rather keep as is.

I approved, but you should probably get an approval from a code owner too.

@tal-sapan
Copy link

Also, I didn't go over the tests for the new package

@bd82
Copy link
Member Author

bd82 commented Jan 20, 2021

I asked @saggir to review as well.

@bd82 bd82 merged commit 0820834 into master Jan 31, 2021
@bd82 bd82 deleted the wrapper branch January 31, 2021 11:49
bd82 added a commit that referenced this pull request Feb 7, 2021
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