Skip to content

update dependencies to latest#88

Merged
TimMcCauley merged 1 commit intomasterfrom
update-deps
Oct 19, 2021
Merged

update dependencies to latest#88
TimMcCauley merged 1 commit intomasterfrom
update-deps

Conversation

@nilsnolde
Copy link
Copy Markdown
Collaborator

fixes #87

was waiting for some process to finish, so I quickly updated the deps. next docker release might fail otherwise too..

I checked with vroom master: health endpoint works, but didn't check any router request.

@nilsnolde nilsnolde requested a review from jcoupey October 19, 2021 11:30
Comment thread src/config.js
let config_yml;
try {
config_yml = yaml.safeLoad(fs.readFileSync('./config.yml'));
config_yml = yaml.load(fs.readFileSync('./config.yml'));
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

safeLoad is deprecated

Comment thread src/config.js

let baseurl = config_yml.cliArgs.baseurl;
if (baseurl.substr(-1) != '/') {
if (baseurl.substr(-1) !== '/') {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

implicit conversion warning

Comment thread src/index.js
const accessLogStream = rfs.createStream(args.logdir + '/access.log', {
compress: 'gzip',
size: args.logsize
size: args.logsize,
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

lots of prettier warnings fixed

@nilsnolde nilsnolde requested a review from TimMcCauley October 19, 2021 11:31
@TimMcCauley TimMcCauley merged commit ae28030 into master Oct 19, 2021
@TimMcCauley
Copy link
Copy Markdown
Collaborator

LGTM, ty

@jcoupey jcoupey added this to the v0.10.0 milestone Oct 19, 2021
@nilsnolde
Copy link
Copy Markdown
Collaborator Author

didn't check router requests yet: valhalla worked, so should be all fine

@jcoupey
Copy link
Copy Markdown
Contributor

jcoupey commented Oct 19, 2021

On an Ubuntu 20.04 box, I'm getting warnings on the node version:

npm WARN notsup Unsupported engine for husky@7.0.2: wanted: {"node":">=12"} (current: {"node":"10.19.0","npm":"6.14.5"})
npm WARN notsup Not compatible with your version of node/npm: husky@7.0.2
npm WARN notsup Unsupported engine for commander@8.2.0: wanted: {"node":">= 12"} (current: {"node":"10.19.0","npm":"6.14.5"})
npm WARN notsup Not compatible with your version of node/npm: commander@8.2.0

This was the reason why I did not update husky back in #64. What's the implication for users with node 10?

@nilsnolde
Copy link
Copy Markdown
Collaborator Author

ouch.. didn't test it (and frankly didn't think about it), but will do so quickly with a docker image. I'm on v14.17.4. we can look into having a quick github actions check, at least for ubuntu-20.04? arch linux just isn't the safest dev env;)

@nilsnolde
Copy link
Copy Markdown
Collaborator Author

fwiw, in fact node v10 is unsupported (not even security fixes anymore), v12 is the current LTR: https://endoflife.date/nodejs. but out-of-the-box ubuntu-20.04 support has of course priority

@jcoupey
Copy link
Copy Markdown
Contributor

jcoupey commented Oct 19, 2021

out-of-the-box ubuntu-20.04 support has of course priority

That's my point since running on latest Ubuntu LTS is a de facto standard for many users.

Maybe this only means downgrading a couple dependencies to suppress warnings?

TimMcCauley pushed a commit that referenced this pull request Oct 19, 2021
@TimMcCauley
Copy link
Copy Markdown
Collaborator

Sorry for premature merge, it is probably not worth it (yet...) happy both if we revert?

@nilsnolde
Copy link
Copy Markdown
Collaborator Author

can happen. it's necessary to upgrade, there's a dependency conflict, it's what this PR is fixing, see #87 . about to open another one

@nilsnolde nilsnolde mentioned this pull request Oct 19, 2021
@jcoupey jcoupey deleted the update-deps branch October 20, 2021 07:48
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.

Unresolvable dependencies

3 participants