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

Update Knex dep to allow 0.20.x #2049

Merged
merged 2 commits into from Jan 19, 2020
Merged

Conversation

mastermatt
Copy link
Contributor

@mastermatt mastermatt commented Jan 18, 2020

https://github.com/knex/knex/blob/master/CHANGELOG.md#0200---25-october-2019

Includes a small lint fix and updates package.json>engines>node to specify 8+.
I looks like Bookshelf already dropped support for Node 6 w/ 735f2ac, but this config was forgotten.

Fixes #2048

@mastermatt mastermatt requested a review from ricardograca Jan 18, 2020
https://github.com/knex/knex/blob/master/CHANGELOG.md#0200---25-october-2019

Includes a small lint fix and updates package.json>engines>node to specify 8+.
I looks like Bookshelf already dropped support for Node 6 w/ 735f2ac,
but this config was forgotten.
@ricardograca
Copy link
Member

@ricardograca ricardograca commented Jan 18, 2020

The change in minimum supported Node version is a bit troublesome since it's a breaking change. Let me think about this for a few more days.

@mastermatt
Copy link
Contributor Author

@mastermatt mastermatt commented Jan 18, 2020

Be aware that Knex dropped support for Node 6 back in 0.18.0.
Bookshelf adopted 0.18 in #1991 before 1.0.0 was cut, so it seems the current major version of Bookshelf already only supports 8, 10, 12, which was also reflected in the changes made in the Travis config in that same PR.
So this isn't a breaking change.

@ricardograca
Copy link
Member

@ricardograca ricardograca commented Jan 19, 2020

Right, that's a good point, but Bookshelf still supports older versions of Knex as you can see in peerDependencies, so it is possible to make Bookshelf work on Node.js 6 if you install the correct version of Knex.

I'll create a review to reflect these points.

Copy link
Member

@ricardograca ricardograca left a comment

We still support Node.js 6 technically, although not very actively.

package.json Outdated
@@ -85,6 +85,6 @@
"license": "MIT",
"readmeFilename": "README.md",
"engines": {
"node": ">=6"
"node": ">=8"
Copy link
Member

@ricardograca ricardograca Jan 19, 2020

Choose a reason for hiding this comment

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

Can you remove this change? It's not correct since it's still possible to run on Node 6 if you install the correct version of Knex as listed in peerDependencies.

Copy link
Contributor Author

@mastermatt mastermatt Jan 19, 2020

Choose a reason for hiding this comment

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

yep. I put it back.

@ricardograca ricardograca added this to In progress in Version 1.x.x via automation Jan 19, 2020
@ricardograca ricardograca removed this from In progress in Version 1.x.x Jan 19, 2020
Copy link
Member

@ricardograca ricardograca left a comment

Looks good. Thanks for the contribution.

@ricardograca ricardograca merged commit d35c6d8 into bookshelf:master Jan 19, 2020
2 checks passed
@mastermatt mastermatt deleted the allow-knex-0-20x branch Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants