Skip to content

fix: enableVerbose() not setting isVerbose correctly#140

Merged
jankapunkt merged 4 commits intometeorrn:devfrom
taras-danyliuk:master
Dec 22, 2023
Merged

fix: enableVerbose() not setting isVerbose correctly#140
jankapunkt merged 4 commits intometeorrn:devfrom
taras-danyliuk:master

Conversation

@taras-danyliuk
Copy link
Copy Markdown
Contributor

Summary

Fix broken Meteor.enableVerbose() due to setting isVerbose in wrong scope.

Involved parts of the project

pretty much all parts :)

Targeted Meteor release version

doesn't affect communication with Meteor

Reproduction

Currently Meteor.enableVerbose() doesn't log extra info.
With this fix Meteor.enableVerbose() brings back all console.warn logs

@jankapunkt jankapunkt changed the base branch from master to dev December 21, 2023 13:29
@jankapunkt
Copy link
Copy Markdown
Member

Thank you for pointing this out! If I get it correct then you aim to make the Meteor.isVerbose calls to work when used in the other files, right? This is good but seems to cause the logs in the Meteor file to not work anymore, because there is often if (isVerbose) { using the local variable.

This is not your fault, but due to bad design (using a local variable and a property at the same time to determine if is verbose).

In order to make really all verbosity statements work I propose the following changes:

  • the local variable should be removed
  • the enableVerbose should be flagged as /** @deprecated */ but kept in, to avoid breaking things
  • all checks that use the local variable, such as if (isVerbose) { should also use if (this.isVerbose) {.

Would you mind updating your pull request accordingly?

@taras-danyliuk
Copy link
Copy Markdown
Contributor Author

I don't think it is needed to mark enableVerbose as deprecated. I didn't notice isVerbose usage as local variable, let me check and correct it

@jankapunkt jankapunkt merged commit ed9c0f6 into meteorrn:dev Dec 22, 2023
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