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.

Sign up for GitHub

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

Jump to bottom

Preemptively unblock Node 20 #659

Merged
merged 1 commit into from
Mar 2, 2023
Merged

Preemptively unblock Node 20 #659

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion azure-pipelines/e2e-integration-test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
variables: {
NODE_14: '14.x',
NODE_16: '16.x',
NODE_18: '18.x'
NODE_18: '18.x',
NODE_20: '19.x' # Temporarily using v19 until v20 is released
}

pr: none
Expand All @@ -19,6 +20,9 @@ strategy:
UBUNTU_NODE18:
IMAGE_TYPE: 'ubuntu-latest'
NODE_VERSION: $(NODE_18)
UBUNTU_NODE20:
IMAGE_TYPE: 'ubuntu-latest'
NODE_VERSION: $(NODE_20)
WINDOWS_NODE14:
IMAGE_TYPE: 'windows-latest'
NODE_VERSION: $(NODE_14)
Expand All @@ -28,6 +32,9 @@ strategy:
WINDOWS_NODE18:
IMAGE_TYPE: 'windows-latest'
NODE_VERSION: $(NODE_18)
WINDOWS_NODE20:
IMAGE_TYPE: 'windows-latest'
NODE_VERSION: $(NODE_20)
MAC_NODE14:
IMAGE_TYPE: 'macOS-latest'
NODE_VERSION: $(NODE_14)
Expand All @@ -37,6 +44,9 @@ strategy:
MAC_NODE18:
IMAGE_TYPE: 'macOS-latest'
NODE_VERSION: $(NODE_18)
MAC_NODE20:
IMAGE_TYPE: 'macOS-latest'
NODE_VERSION: $(NODE_20)
pool:
vmImage: $(IMAGE_TYPE)
steps:
Expand Down
21 changes: 20 additions & 1 deletion azure-pipelines/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
variables: {
NODE_14: '14.x',
NODE_16: '16.x',
NODE_18: '18.x'
NODE_18: '18.x',
NODE_20: '19.x' # Temporarily using v19 until v20 is released
}

pr:
Expand All @@ -25,6 +26,9 @@ jobs:
UBUNTU_NODE18:
IMAGE_TYPE: 'ubuntu-latest'
NODE_VERSION: $(NODE_18)
UBUNTU_NODE20:
IMAGE_TYPE: 'ubuntu-latest'
NODE_VERSION: $(NODE_20)
WINDOWS_NODE14:
IMAGE_TYPE: 'windows-latest'
NODE_VERSION: $(NODE_14)
Expand All @@ -34,6 +38,9 @@ jobs:
WINDOWS_NODE18:
IMAGE_TYPE: 'windows-latest'
NODE_VERSION: $(NODE_18)
WINDOWS_NODE20:
IMAGE_TYPE: 'windows-latest'
NODE_VERSION: $(NODE_20)
MAC_NODE14:
IMAGE_TYPE: 'macOS-latest'
NODE_VERSION: $(NODE_14)
Expand All @@ -43,6 +50,9 @@ jobs:
MAC_NODE18:
IMAGE_TYPE: 'macOS-latest'
NODE_VERSION: $(NODE_18)
MAC_NODE20:
IMAGE_TYPE: 'macOS-latest'
NODE_VERSION: $(NODE_20)
pool:
vmImage: $(IMAGE_TYPE)
steps:
Expand Down Expand Up @@ -80,6 +90,9 @@ jobs:
UBUNTU_NODE18:
IMAGE_TYPE: 'ubuntu-latest'
NODE_VERSION: $(NODE_18)
UBUNTU_NODE20:
IMAGE_TYPE: 'ubuntu-latest'
NODE_VERSION: $(NODE_20)
WINDOWS_NODE14:
IMAGE_TYPE: 'windows-latest'
NODE_VERSION: $(NODE_14)
Expand All @@ -89,6 +102,9 @@ jobs:
WINDOWS_NODE18:
IMAGE_TYPE: 'windows-latest'
NODE_VERSION: $(NODE_18)
WINDOWS_NODE20:
IMAGE_TYPE: 'windows-latest'
NODE_VERSION: $(NODE_20)
MAC_NODE14:
IMAGE_TYPE: 'macOS-latest'
NODE_VERSION: $(NODE_14)
Expand All @@ -98,6 +114,9 @@ jobs:
MAC_NODE18:
IMAGE_TYPE: 'macOS-latest'
NODE_VERSION: $(NODE_18)
MAC_NODE20:
IMAGE_TYPE: 'macOS-latest'
NODE_VERSION: $(NODE_20)
pool:
vmImage: $(IMAGE_TYPE)
steps:
Expand Down
2 changes: 1 addition & 1 deletion src/nodejsWorker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
const logPrefix = 'LanguageWorkerConsoleLog';
const errorPrefix = logPrefix + '[error] ';
const warnPrefix = logPrefix + '[warn] ';
const supportedVersions: string[] = ['v14', 'v16', 'v18'];
const supportedVersions: string[] = ['v14', 'v16', 'v18', 'v20'];
const devOnlyVersions: string[] = ['v15', 'v17', 'v19'];
let worker;

Expand Down
7 changes: 6 additions & 1 deletion test/eventHandlers/WorkerInitHandler.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import * as escapeStringRegexp from 'escape-string-regexp';
import 'mocha';
import { ITestCallbackContext } from 'mocha';
import * as mockFs from 'mock-fs';
import * as semver from 'semver';
import { AzureFunctionsRpcMessages as rpc } from '../../azure-functions-language-worker-protobuf/src/rpc';
import { logColdStartWarning } from '../../src/eventHandlers/WorkerInitHandler';
import { WorkerChannel } from '../../src/WorkerChannel';
Expand Down Expand Up @@ -218,14 +219,18 @@ describe('WorkerInitHandler', () => {
},
});

const jsonError = semver.gte(process.versions.node, '19.0.0')
Copy link
Contributor

Choose a reason for hiding this comment

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

I support the idea of this PR to preemptively support node 20 as much as possible. But I worry, are we adding additional unnecessary burden here by using node 19 as a proxy for node 20? Are we forcing ourselves to support features/updates in node 19 that may not end up making it to node 20? This change is an example. Are we sure that node 20 will still have this different behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well...it took me all of 10 minutes to fix this unit test. Can you expand on what you mean by burden?

For the record, this PR doesn't mean we actually support anything. Users have to wait for official public preview/GA announcements.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, I'm just saying in general. I'm not sure what other changes in node 19 that might require us to update/modify tests that won't be necessary in node 20. If you don't think this is a significant risk, I'm cool merging this and won't block on it

? 'Unexpected token \'g\', "gArB@g3 dAtA" is not valid JSON'
: 'Unexpected token g in JSON at position 0';

stream.addTestMessage(Msg.init(appDir));
await stream.assertCalledWith(
Msg.receivedInitLog,
Msg.warning(
`Worker failed to load package.json: file content is not valid JSON: ${path.join(
appDir,
'package.json'
)}: Unexpected token g in JSON at position 0`
)}: ${jsonError}`
),
Msg.response
);
Expand Down