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

Crash debug logging using loglevel library #123

Merged
merged 5 commits into from
Jan 22, 2022
Merged

Conversation

eonwarped
Copy link
Contributor

Added config parameter to tweak log level to help in crash scenarios.

Copy link
Collaborator

@bt-cryptomancer bt-cryptomancer left a comment

Choose a reason for hiding this comment

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

The actual code changes look good, however I noticed you updated the package-lock.json file to use lockfileVersion 2. I believe this version 2 format requires a newer version of npm. Introducing the loglevel library shouldn't change the minimum version of npm required to run the node, right? So to avoid affecting compatibility with older versions of npm, I'd prefer to have package-lock.json reverted back to the previous lockfileVersion 1 format. Unless you feel strongly about having it be lockfileVersion 2 for some reason.

@4Ykw
Copy link

4Ykw commented Jan 5, 2022

Tested myself these for 8 days at least. So far no issues.

@bt-cryptomancer - lockfileVersion 2 requires npm 7 at least and is backward compatible with version 1. But if we want to support npm 5 or 6, then we need to keep lockfileVersion 1.
https://docs.npmjs.com/cli/v7/configuring-npm/package-lock-json

I have been using npm ci and so far I still have this being respected. Using npm 7.5.1

@eonwarped
Copy link
Contributor Author

Yeah, I think it just automatically updated the lockfile version when I did npm install . Don't think it changes node version compatibility. I think it's fine to just have people use the newer version of npm.

Copy link
Collaborator

@bt-cryptomancer bt-cryptomancer left a comment

Choose a reason for hiding this comment

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

Okay, I did a bit of reading about lockfileVersion 2 and noticed that it is backwards compatible with npm 6. I guess it doesn't really matter which version we use, and the underlying Node.js compatibility doesn't change either. So if you guys have no concerns about it, then I'm fine with it too.

Happy to approve this PR now.

@bt-cryptomancer bt-cryptomancer merged commit 8809d5e into hive-engine Jan 22, 2022
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