try supporting node 14 on v5 #4280
Conversation
Overall package sizeSelf size: 6.46 MB Dependency sizes
🤖 This report was automatically generated by heaviest-objects-in-the-universe |
BenchmarksBenchmark execution time: 2024-05-06 18:39:52 Comparing candidate commit f9a0167 in PR branch Found 2 performance improvements and 0 performance regressions! Performance is the same for 258 metrics, 6 unstable metrics. scenario:plugin-graphql-with-depth-and-collapse-on-18
scenario:plugin-graphql-with-depth-on-max-18
|
bengl
force-pushed
the
bengl/try-supporting-14
branch
from
334feaa
to
ceb4bad
Compare
bengl
force-pushed
the
bengl/try-supporting-14
branch
from
df9628b
to
579f5c5
Compare
So specifically any release line that we've ever supported for the entire history of the project? As opposed to any actively maintained release line? |
| @@ -943,7 +959,7 @@ jobs: | |||
| - uses: ./.github/actions/testagent/start | |||
| - uses: ./.github/actions/node/setup | |||
| - run: yarn install | |||
| - uses: ./.github/actions/node/oldest | |||
| - uses: ./.github/actions/node/18 | |||
There was a problem hiding this comment.
Doesn't that make it so that we don't know if we support Node 14 for Next? (and other similar ones)
There was a problem hiding this comment.
Correct. In these cases, the tests do not pass on older Node.js versions, so we can't verify (or perhaps even claim) support. Arguably, we never could.
| @@ -33,7 +33,7 @@ jobs: | |||
| integration-ci: | |||
| strategy: | |||
| matrix: | |||
| version: [18, latest] | |||
| version: [16, 18, 20, latest] # node 14 does not have experimental-fetch | |||
There was a problem hiding this comment.
How does this work on v3 branch?
There was a problem hiding this comment.
IIRC it doesn't, and this test fails on the v3 branch.
.github/workflows/tracing.yml
Outdated
| @@ -13,7 +13,7 @@ concurrency: | |||
|
|
|||
| jobs: | |||
| macos: | |||
| runs-on: macos-latest | |||
| runs-on: macos-latest-large | |||
There was a problem hiding this comment.
This uses a more recent version that the version we currently guarantee to work on (which granted was already the case before, but that's also why things were failing)
| @@ -67,7 +67,7 @@ | |||
| }, | |||
| "homepage": "https://github.com/DataDog/dd-trace-js#readme", | |||
| "engines": { | |||
| "node": ">=18" | |||
| "node": ">=14" | |||
There was a problem hiding this comment.
This check and checks on the dd-trace version are also in a few places in code which might need to be updated as well which I don't see in this PR.
There was a problem hiding this comment.
The only place engines appears to be used in code is scripts/preinstall.js, which doesn't do anything based on particular version, just check compat, so it should be fine.
The only places the Node.js version is inspected appear to be legitimate checks on the version, not taking into account the dd-trace version at all.
The only places the tracer version (e.g. also DD_MAJOR and friends) is used seem to be places where we've done in-code breaking-change switches, which shouldn't be changed by this PR.
There was a problem hiding this comment.
Perfect then, just wanted to make sure nothing was missed 👍
Currently actively maintained release lines. |
Hmm... Since we only support v14 for another 9 days is this worth the effort? |
|
I don't think there is any roadblock from ASM's perspective, as we backport everything anyway. We'd just have to doublecheck our CI testing the right stuff in the right versions. Otherwise SGTM |
What does this PR do?
Adds support for Node.js v14 to the master branch, with intent to add it to a 5.x (and even 4.x) release.
Motivation
As we move toward automated installation methods, larger amounts of Node.js release history support are becoming necessary on the latest release line. For now, the easiest path foward is to ensure we support back to the earliest Node.js version we support on any release line. In the near future, we'll work out a policy around this so that expectations are clear.
Additional Notes