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

feat: upgrade to node18 #5155

Merged
merged 71 commits into from
Oct 20, 2023
Merged

Conversation

gbockus-sf
Copy link
Contributor

What does this PR do?

Upgrade the version of node used for development to Node 18.

It's a Tangled web we weave.
This PR does the following:

  • Update project configs for nvm
  • Update typescript to version 5
    • Required in order to use the node18 configuration for tsc
  • Update the github actions to use node18 for tests/builds
  • Disable the old unit tests in the salesforcedx-utils-vscode package. These were spawning sfdx processes (unit tests) and after inspection I decided it was cleaner to remove them then try to update.
  • Update jest config with commented out config for seeing coverage of each entire project (commented out as it slows down CI significantly)

What issues does this PR fix or reference?

Note the node18 upgrade was broken into several tickets
@W-13209367@
@W-14196469@
@W-14196410@
@W-14196482@
@W-14196589@

Notes

For testing this change ensure that you are on node 18 and npm 9.8
My sure fire way to get everything to build happily is to follow the following steps:

  • git clean -xdf
  • npm install
  • npm run compile
    #success

@peternhale peternhale self-requested a review October 16, 2023 17:57
Copy link
Contributor

@peternhale peternhale left a comment

Choose a reason for hiding this comment

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

@gbockus-sf I posted a number questions. Looks good overall, I will try to test tomorrow morning, if that is ok.

@@ -45,7 +45,7 @@
"watch": "tsc -watch -p .",
"clean": "shx rm -rf node_modules && shx rm -rf out && shx rm -rf coverage && shx rm -rf .nyc_output",
"test": "npm run test:unit && npm run test:integration",
"test:unit": "jest --coverage",
"test:unit": "jest --coverage --forceExit",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the permanent solution?

@@ -7,7 +7,7 @@

// tslint:disable-next-line:no-var-requires
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the tslint annotations be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch

@@ -23,16 +23,16 @@
"@salesforce/ts-sinon": "1.4.0",
"@types/chai": "4.3.3",
"@types/cross-spawn": "6.0.0",
"@types/jest": "28.1.7",
"@types/jest": "28.1.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a v29* version of this module. Upgrade this with jest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll ensure the jest & @types/jest are all aligned

this.timeout(10000);
await coreExtension.activate();
expect(coreExtension.isActive);
});

it('aura activation', async function() {
// tslint:disable-next-line:no-invalid-this
this.timeout(10000);
Copy link
Contributor

Choose a reason for hiding this comment

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

You could try it('foo bar baz).timeout(10000);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

interesting. Didn't know that was a thing. I'll give it a shot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bummer
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Configuring the type for this seems like the right path for these weird mocha tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gbockus-sf the timeout goes on the it not the expect

it('foo bar`, () => {
  expect(....);
}).timeout(10000);

@gbockus-sf
Copy link
Contributor Author

Did some initial testing on the vsix builds. I see a couple of issues with the LWC & lighting language servers. Will see what I can do to remedy this evening

Node.js v18.15.0
[Info  - 4:58:53 PM] Connection to server got closed. Server will restart.
node:internal/modules/cjs/loader:1132
  throw err;
  ^

Error: Cannot find module '/Users/gbockus/.vscode-insiders/node_modules/@salesforce/lwc-language-server/lib/server.js'
    at Module._resolveFilename (node:internal/modules/cjs/loader:1129:15)
    at Module._load (node:internal/modules/cjs/loader:974:27)
    at f._load (node:electron/js2c/asar_bundle:2:13330)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:87:12)
    at node:internal/main/run_main_module:23:47 {
  code: 'MODULE_NOT_FOUND',
  requireStack: []
}```

Results in those two extension not activating. 

@gbockus-sf
Copy link
Contributor Author

FYI @peternhale ☝️

"aura-language-server",
"lib",
"server.js"
],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the deal here is the server file lives in the root node_modules directory when running in debug mode as part of the monorepo, but lives in the extension level node_module directory when running as a vsix. I updated both the lightning and lwc extension to account for that change as part of the packaging. Part of this is updating where each extension gets the path to the server to follow the same pattern we see in other extensions.

@gbockus-sf
Copy link
Contributor Author

vsix issue with the lightning/lwc lsp resolved with commit 16fa698

@gbockus-sf gbockus-sf merged commit 6736855 into develop Oct 20, 2023
13 checks passed
@gbockus-sf gbockus-sf deleted the gbockus/W-13209367-upgrade-node-18 branch October 20, 2023 20:56
This was referenced Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants