feat!: Require Node >=18 as minimum supported version
#14749
Conversation
mydea
force-pushed
the
fn/bump-node-18
branch
from
e8d4c16 to
a711f21
Compare
| @@ -35,6 +35,7 @@ | |||
| "@angular/platform-browser": "^14.3.0", | |||
| "@angular/platform-browser-dynamic": "^14.3.0", | |||
| "@angular/router": "^14.3.0", | |||
| "@types/node": "^14.8.0", | |||
There was a problem hiding this comment.
The build fails without this, also with v16 of @types/node :( cc @Lms24
There was a problem hiding this comment.
yeah, I added this here and it seems fine, just wanted to call it out. if we revisit this/bump angular, we can maybe drop this (??), let's see!
There was a problem hiding this comment.
Ah sorry, I missed that this had to be added explicitly. But yeah, I think it's reasonable for us to use 14 as long as we stay with Angular 14 as the min version (Angular 14 still used Node 14 as its minimal version). We can and should reevaluate once we need to drop Angular 14 support. To get up to 18.19.1 as the min supported version we'd however need to drop everything below Angular 18, which is a pretty big jump
There was a problem hiding this comment.
yeah, it's not a big deal to leave this in I'd say, for now!
| @@ -89,7 +89,7 @@ export function parseProxyResponse(socket: Readable): Promise<{ connect: Connect | |||
| return; | |||
| } | |||
|
|
|||
| const headerParts = buffered.slice(0, endOfHeaders).toString('ascii').split('\r\n'); | |||
| const headerParts = buffered.subarray(0, endOfHeaders).toString('ascii').split('\r\n'); | |||
There was a problem hiding this comment.
buffer.slice is deprecated since Node 16.
| @@ -52,7 +52,7 @@ export class AsyncLocalStorageContextManager extends AbstractAsyncHooksContextMa | |||
| getStore() { | |||
| return undefined; | |||
| }, | |||
| run(_store, callback, ...args) { | |||
| run(_store: unknown, callback: () => Context, ...args: unknown[]) { | |||
There was a problem hiding this comment.
not sure why this is not inferred anymore, but this was missing now.
mydea
force-pushed
the
fn/bump-node-18
branch
from
ec1dfef to
5c5b240
Compare
|
It's worth noting that this would take our minimum supported Electron version from v15 to v29 |
mydea
changed the title
>=18.19.1 as minimum supported version>=18 as minimum supported version
I adjusted this to be 18+ instead, which should broaden this a bit more? |
|
Yep, that would take the minimum supported Electron version from v15 to v23 |
mydea
force-pushed
the
fn/bump-node-18
branch
from
ef987d0 to
58d0261
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
mydea
requested review from
timfish,
lforst,
AbhiPrasad,
krystofwoldrich and
Lms24
There was a problem hiding this comment.
We need to update https://github.com/getsentry/sentry-javascript/blob/develop/packages/profiling-node/README.md#prebuilt-binaries to start at v18 now
Feels good that we can progress further with #11084 now
| @@ -58,6 +58,7 @@ describe('GCPFunction', () => { | |||
| headers = { ...headers, ...trace_headers }; | |||
| } | |||
| return new Promise((resolve, _reject) => { | |||
| // eslint-disable-next-line deprecation/deprecation | |||
| const d = domain.create(); | |||
There was a problem hiding this comment.
l: Can we just rewrite this to not use domains? We can create a GH issue to follow up on it.
mydea
force-pushed
the
fn/bump-node-18
branch
from
7cc4e25 to
80d832f
Compare
❌ 1 Tests Failed:
View the top 1 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
We decided to require node 18+ in v9. This is to align with the upcoming changes in OpenTelemetry. We also believe it is a reasonable version requirement - Node 16 is EOL for some time now. This will also allow us to update a bunch of dev tooling (vitest, ...) properly and streamline some processes.
For users requiring Node 14/16, they can remain on v8 (which will be continued to be supported with bug fixes for some time).
ref #14257