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

Add support for Node.js 12.x #1728

Merged
merged 1 commit into from Apr 30, 2019

Conversation

Projects
None yet
4 participants
@bajtos
Copy link
Member

commented Apr 29, 2019

  • Fix code to pass all tests on Node.js 12
  • Add Node.js 12 to the list of Travis CI platforms

Related issues

  • connect to <link_to_referenced_issue>

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style
    guide

@bajtos bajtos requested review from raymondfeng and strongloop/loopback-3x-maintainers Apr 29, 2019

@bajtos bajtos self-assigned this Apr 29, 2019

@bajtos bajtos added this to the April 2019 milestone milestone Apr 29, 2019

@bajtos

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2019

Any objections against back-porting this change to 3.x?

@@ -389,6 +389,8 @@ ModelUtils._coerce = function(where, options) {
self._coerce(clauses[k], options);
}

where[p] = clauses;

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 29, 2019

Author Member

This is kind of weird. Before my change, the block "handle logical operators" did not apply updates made by coerceArray to the original where[p] value. As a result, array-like object {1: {field: 'value'}} was not converted into an array [{field: 'value'}]. I am not sure why this was not caught earlier and surfaced only with Node.js 12. Is it perhaps a change in how deep-equality checks work on the new Node version? IDK.

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 29, 2019

Author Member

Here are the tests that are failing without my fix:

COERCIONS.forEach(coercion => {
const inStr = JSON.stringify(coercion.in);
it('coerces where clause with array-like objects ' + inStr, () => {
assert.deepEqual(model._coerce(coercion.in), coercion.out);
});
});

Add support for Node.js 12.x
- Fix code to pass all tests on Node.js 12
- Add Node.js 12 to the list of Travis CI platforms

@bajtos bajtos force-pushed the chore/add-node-12 branch from 7ef195c to 5c3405e Apr 29, 2019

@b-admike
Copy link
Member

left a comment

LGTM, I think the changes you made in order to add support are reasonable.

@jannyHou
Copy link
Contributor

left a comment

👍 LGTM when jenkins tests pass.

@bajtos

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2019

@slnode test please

1 similar comment
@b-admike

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

@slnode test please

@bajtos bajtos merged commit 6793be2 into master Apr 30, 2019

13 of 14 checks passed

coverage/coveralls Coverage decreased (-0.004%) to 84.068%
Details
Commit Linter commits are all properly formatted
Details
PR Linter PR is up to date
Details
[cis-jenkins] PR Builder Build finished.
Details
[cis-jenkins] x64 && linux && nvm,10 Success! (5c3405e)
Details
[cis-jenkins] x64 && linux && nvm,8 Success! (5c3405e)
Details
clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
loopback-datasource-juggler Success! (5c3405e)
Details
loopback-datasource-juggler/node=4.x,os=windows Success! (5c3405e)
Details
loopback-datasource-juggler/node=6.x,os=windows Success! (5c3405e)
Details
pr-builder
Details
security/snyk - package.json (StrongLoop) No manifest changes detected

@bajtos bajtos deleted the chore/add-node-12 branch Apr 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.