Fix: #1840 feat: node 15 support #1841
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kevcodez
commented
Oct 28, 2020
•
kevcodez
commented
- Add Node.js 15 to package.json engine field.
- Add Node.js version 15.x to CI build matrix.
|
💚 CLA has been signed |
❕ Build Aborted
Expand to view the summary
Build stats
Steps errors
Expand to view the steps failures
|
kevcodez
changed the title
|
@kevcodez -- thanks for the contribution (and the gentle reminder 😄). The CI errors you're seeing are not Node 15 related -- ioredis released a change that broke support in node 8 (which is their prerogative, of course 😄 ). We've got an issue open to get our CI adjusted. Once we have that adjusted, we can rerun the jobs here and see how Node 15 does. |
trentm
reviewed
Oct 29, 2020
.ci/.jenkins_nodejs.yml
Outdated
| @@ -1,4 +1,6 @@ | |||
| NODEJS_VERSION: | |||
| - "15" | |||
| - "15.0" | |||
There was a problem hiding this comment.
How about dropping "15.0" and just have "15" here? I'm inclined to not bother explicitly supporting the first minor-level release of odd (i.e. non-LTS) node releases (https://github.com/nodejs/Release#release-phases). IMO, the main value of supporting the odd-numbered releases is to (a) prepare for changes coming in the subsequent even-numbered LTS release and (b) to help users of this module that are doing the same.
@astorm Do you have an opinion?
There was a problem hiding this comment.
I don't know if I have a say in this :D However, this seems very reasonable. V15 will not receive any support in 6 months from now anyway
|
jenkins run the tests |
|
@kevcodez Can you force push with an updated commit message to pass the "commitlint" check, please. Something like the following should work I think: |
|
There is a test failure with node v15 that I haven't looked into yet: https://travis-ci.org/github/elastic/apm-agent-nodejs/jobs/739539136 Have you been able to run the tests ( |
|
Force pushed the commit message. |
|
Locally, the test neither runs with Node 14 or 15. |
kevcodez
force-pushed
the
master
branch
3 times, most recently
from
f2aae9d to
31ac0ca
Compare
|
@trentm Node 15 uses NPM 7 which seems to properly check peer dependencies. apm-agent-nodejs uses graphql 15.x and express-graphl 0.9.x (which only supports graphl 14.x). Downgrading to graphql 14.x breaks three tests. Upgrading express-graphql to 0.11.x also breaks tests, could you have a look? |
|
@kevcodez Sorry I dropped the ball here for a while. I'll try to take a look soon. |
|
FWIW, that peerDeps failure from npm 7 looks like this: One potential workaround is https://docs.npmjs.com/cli/v7/using-npm/config#legacy-peer-deps |
|
@kevcodez How about as a start you add this patch to your branch: Also it would be good if you could merge in recent changes from master (or rebase on master, whichever you prefer). After the above patch I still see 3 errors from |
|
@trentm Rebased on master, added your patch. I've seen that the transaction result is Trying to get the tests to run locally... |
|
Failures in "test/instrumentation/modules/http/aborted-requests-enabled.js" with node v15: These are happening because of this change in node v15: https://github.com/nodejs/node/pull/31818/files#diff-48d21edbddb6e855d1ee5716c49bcdc0d913c11ee8a24a98ea7dbc60cd253556L661-R706 @kevcodez Can you add this patch to your branch, please: https://gist.github.com/trentm/8dd8d2e35958d2624f797d53d75c3609 |
|
After that change the regular test suite ( |
|
@trentm Thank you for helping out. 🍻 I was unable to get the tests to run on my Mac so this was impossible to test for me. Rebased with master, applied your diff, squashed the commits. The tests pass. |
|
jenkins run the tests |
|
jenkins run the module tests |
|
@elasticmachine, run elasticsearch-ci/docs |
|
jenkins run the module tests for ALL |
|
Failures with the "hapi" test-all-versions tests: Ouch, a re-run of that shows inconsistency on which hapi versions there are errors with: There is some bug in installing the hapi module when there is already a hapi module installed: The "hapi" module installation is now broken. |
|
SPecifically the error we get from testing is a failure to load "node_modules/hapi/package.json": because npm@7 failed to install that correctly when changing hapi versions. "Silly"-level logging from npm doesn't show anything obvious: I get the same behaviour with the latest npm 7.x installed. My guess is that there is some race in npm when attempting to change "node_modules/hapi" versions and that it is more likely to be hit with the larger set of deps that "hapi" has. I don't think that is a very solid guess because I reliably hit it on both the Jenkins build agent and on my dev mac. It appears to happen when moving between a hapi@17 and hapi@18. I don't see an obviously related issue at: https://github.com/npm/cli/issues?q=is%3Aissue+is%3Aopen+label%3A%22Release+7.x%22 I've opened npm/cli#2267 for this. |
|
I have a patch to work around that npm issue for now: https://gist.github.com/trentm/9103d7ea9f2db2e7ec8fa21ac712f200 @kevcodez Are you able to add this to your branch? |
|
@trentm Rebased on master, applied your patch. |
|
jenkins run the module tests for ALL |
|
Sigh. Next set of failures is the "express" TAV tests: I'm looking. |
|
@kevcodez I noticed your current rebased commit has changed all (or most) uses of single-quotes in ".tav.yml" to double-quotes. Would you be able to revert that change. It adds noise to the diff. Was that done by some normalization in your editor, perhaps? |
|
The TAV=express failure is in this test: This happens from node's internals blowing this assert: https://github.com/nodejs/node/blob/v15.2.1/lib/_http_server.js#L243
Condition #1 happens with an Express app is responding to a HEAD request for express versions 4.6.0 to 4.10.4. The change in node v15 that changed the behaviour of asserting on double-calling of Here is an example simple node.js app ("foo.js") that demonstrates being able to hit that internal assertion: Start the app: then call it: and it will exit with that internal assertion: However if you use node v14 for the app, this internal assertion is not hit. Likewise this internal assertion is not hit if the APM instrumentation is not enabled. Instead, a separate "write after end" error is hit: I need to dig into this more to understand what part of the APM instrumentation is causing this difference. |
|
I tracked this down a little bit, and I believe it is related to the "end-of-stream" usage in the Node.js agent's "lib/instrumentation/http-shared.js". My next TODO is to create a small repro case that does not include this full agent. |
|
Related: elastic/apm-nodejs-http-client#124 |
1 task
|
@kevcodez Sorry this has languished. I will likely open a separate PR soon to add this and (now) node v16 support -- crediting your start here. |
6 tasks
|
FWIW, the double- |
4 tasks
trentm
added a commit
that referenced
this pull request
Apr 30, 2021
#2055) This borrows work from @kevcodez and myself on #1841. - enable node 16/15 testing for the GH Actions and Jenkins tests - ensure OpenSSL SECLEVEL=0 for tests, required for http2.js test to pass in node:16 docker image - fix hang with POST-handler + @hapi/[email protected] + node v16; we can just move on to hapi v20 - switching to hapi@20 means we need to skip out of hapi tests for node v10 and below Fixes: #1840
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.