Skip to content

Conversation

scottinet
Copy link
Contributor

@scottinet scottinet commented Sep 12, 2019

Description

More and more NPM modules are dropping support for Node.js 6, making it hazardous to not test this SDK against that version of node to guarantee that it stays compatible with Kuzzle v1.

Also, Object.entries have been added to this SDK, which does not exist in node6, making Kuzzle crash if an attempt is made to use this SDK with Node.js 6.

So this PR forces Travis to run tests using Node.js 6, and all other changes are made to make that SDK fully-compatible, tests included.

Once Kuzzle v2 is out, we'll release a major version of this SDK dropping support for node 6, allowing us to once again use async/await and other new features.

Other changes

  • fix documentation snippets tests to make them work with kuzzle 1.10

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Sep 12, 2019

This pull request fixes 1 alert when merging 579e51b into 15b77fe - view on LGTM.com

fixed alerts:

  • 1 for Missing variable declaration

@codecov
Copy link

codecov bot commented Sep 12, 2019

Codecov Report

Merging #446 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #446      +/-   ##
==========================================
+ Coverage   95.96%   95.97%   +<.01%     
==========================================
  Files          33       33              
  Lines        1637     1641       +4     
==========================================
+ Hits         1571     1575       +4     
  Misses         66       66
Impacted Files Coverage Δ
src/protocols/http.js 90.6% <100%> (+0.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 15b77fe...fc01ea9. Read the comment docs.

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Sep 12, 2019

This pull request fixes 1 alert when merging 96125e3 into 15b77fe - view on LGTM.com

fixed alerts:

  • 1 for Missing variable declaration

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Sep 12, 2019

This pull request fixes 1 alert when merging fc01ea9 into 15b77fe - view on LGTM.com

fixed alerts:

  • 1 for Missing variable declaration

}
}
}
}' kuzzle:7512/roles/privileged/_create
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we need to create the role beforehand because now kuzzle refuses to create a profile based on a non-existing role

template: default
expected: ^(Kuzzle Server configuration:) {("dump":{.*}),("limits":{.*}),("plugins":{.*}),("queues":{.*}),("repositories":{.*}),("server":{.*}),("services":{.*}),("stats":{.*}),("validation":{.*}),("_":.*),("internal":{.*}),("version":"[0-9]\.[0-9]\.[0-9]")}$

expected: ^(Kuzzle Server configuration:) {("dump":{.*}),("limits":{.*}),("plugins":{.*}),("queues":{.*}),("repositories":{.*}),("server":{.*}),("services":{.*}),("stats":{.*}),("validation":{.*}),("_":.*),("internal":{.*}),("version":"[0-9]+\.[0-9]+\.[0-9]+")}$
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the change here is not immediately obvious: this fixes the erroneous version regex that fails now because, kuzzle being in 1.10, the minor version has 2 digits


for (const [controller, definition] of Object.entries(this.customRoutes)) {
for (const [action, route] of Object.entries(definition)) {
for (const controller of Object.keys(this.customRoutes)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Object.entries does not exist in node6

actions: {
list: true
}
const response = await kuzzle.security.updateRole('read-only', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing await

Copy link
Contributor

@Aschen Aschen left a comment

Choose a reason for hiding this comment

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

Fix the missing await and 👍 for me

@scottinet scottinet merged commit 1c3032d into master Sep 12, 2019
@scottinet scottinet deleted the node6-compat branch September 12, 2019 13:38
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.

3 participants