Be consistent about casing in Query History menu #1369

elenatanasoiu · 2022-05-31T16:39:45Z

Reported here: https://github.com/github/code-scanning/issues/6008

We originally started out by capitalizing each word 1, but made some
small changes 2 which resulted in our Query History options being
inconsistent.

Let's fix that.

Questions

TL;DR: are we comfortable with pinning the node version in an .nvmrc file?

Building the extension: I've followed the contribution docs to do this using npm build. However, I could only complete the build successfully once I downgraded to node v16.14.2. For node v18 the build fails. Would we be okay with adding a .nvmrc file in this repo?

To provide a bit more context: I only found the correct version by spinning up a codespace and snooping around. "Why don't you build the extension in a codespace?" you ask. While it does build successfully, it doesn't seem like you're able to then install the resulting .vsix file into VSCode. Happy to be told otherwise.

Checklist

CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
Issues have been created for any UI or other user-facing changes made by this pull request.
[Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

aeisenberg

Thanks for the PR!

extensions/ql-vscode/package.json

aeisenberg · 2022-05-31T17:31:25Z

Hmmm...I wasn't aware that the extension wasn't building in node v18. It's definitely worth fixing eventually, but in general, we need to stay in sync with the node version shipped with vscode (currently v16.13.0).

I think it is fine to add a .nvmrc file. Also, we should update the engine field in package.json to include a max version as well as min. Using ^16.11.25 should be fine since it allows for anything less than 17.0.0.

Would you be able to raise an issue for the broken build on v18? It's something we should fix before vscode moves to v18.

Reported here: github/code-scanning#6008 We originally started out by capitalizing each word [1], but made some small changes [2] which resulted in our Query History options being inconsistent. Let's fix that. [1]: https://github.com/github/vscode-codeql/blame/a5da5564969a0f37f95f1fe8b856f60810fc5caa/extensions/ql-vscode/package.json [1]: b470e41

charisk · 2022-06-01T08:53:21Z

Seems a bit silly but do we need to update the CHANGELOG file? The PR description check-list suggests yes. cc @shati-patel

shati-patel · 2022-06-01T09:06:56Z

Seems a bit silly but do we need to update the CHANGELOG file? The PR description check-list suggests yes. cc @shati-patel

No harm in updating the changelog, but it's definitely not necessary in this case since it's such a minor change!

elenatanasoiu · 2022-06-01T11:07:51Z

Thanks @charisk & @shati-patel. Have added a line to the changelog.

shati-patel

✅

As recommended here [1], since the current build for this extension does not work with Node v18 [2], it would be good to set a maximum node version until this gets addressed. So we're updating `engines` here to allow for a maximum version, which in this case is v17.0.0. [1]: #1369 (comment) [2]: #1373

As recommended here [1], we want to stay in sync with the current node version shipped with VSCode (16.13.0) [2]. For this we can add a `.nvmrc` file to alert nvm to switch to the preferred version automatically. It will also help prevent builds from failing when setting up the project for the first time, as building the extension currently fails in Node v18. [3] [1]: #1369 (comment) [2]: https://github.com/microsoft/vscode/blob/32d40cf44e893e87ac33ac4f08de1e5f7fe077fc/remote/.yarnrc#L2 [3]: #1373

As recommended here [1], since the current build for this extension does not work with Node v18 [2], it would be good to set a maximum node version until this gets addressed. So we're updating `engines` here to allow for a maximum version, which in this case is v17.0.0. [1]: #1369 (comment) [2]: #1373

As recommended here [1], we want to stay in sync with the current node version shipped with VSCode (16.13.0) [2]. For this we can add a `.nvmrc` file to alert nvm to switch to the preferred version automatically. It will also help prevent builds from failing when setting up the project for the first time, as building the extension currently fails in Node v18. [3] [1]: #1369 (comment) [2]: https://github.com/microsoft/vscode/blob/32d40cf44e893e87ac33ac4f08de1e5f7fe077fc/remote/.yarnrc#L2 [3]: #1373

As recommended here [1], we want to stay in sync with the current node version shipped with VSCode (16.13.0) [2]. For this we can add a `.nvmrc` file to alert nvm to switch to the preferred version automatically. It will also help prevent builds from failing when setting up the project for the first time, as building the extension currently fails in Node v18. [3] We're also updating the docs to mention using `nvm` to manage node versions. [1]: #1369 (comment) [2]: https://github.com/microsoft/vscode/blob/32d40cf44e893e87ac33ac4f08de1e5f7fe077fc/remote/.yarnrc#L2 [3]: #1373

As recommended here [1], since the current build for this extension does not work with Node v18 [2], it would be good to set a maximum node version until this gets addressed. So we're updating `engines` here to allow for a maximum version, which in this case is v17.0.0. [1]: #1369 (comment) [2]: #1373

As recommended here [1], we want to stay in sync with the current node version shipped with VSCode (16.13.0) [2]. For this we can add a `.nvmrc` file to alert nvm to switch to the preferred version automatically. It will also help prevent builds from failing when setting up the project for the first time, as building the extension currently fails in Node v18. [3] We're also updating the docs to mention using `nvm` to manage node versions. [1]: #1369 (comment) [2]: https://github.com/microsoft/vscode/blob/32d40cf44e893e87ac33ac4f08de1e5f7fe077fc/remote/.yarnrc#L2 [3]: #1373

As recommended here [1], since the current build for this extension does not work with Node v18 [2], it would be good to set a maximum node version until this gets addressed. So we're updating `engines` here to allow for a maximum version, which in this case is v17.0.0. [1]: #1369 (comment) [2]: #1373

As recommended here [1], we want to stay in sync with the current node version shipped with VSCode (16.13.0) [2]. For this we can add a `.nvmrc` file to alert nvm to switch to the preferred version automatically. It will also help prevent builds from failing when setting up the project for the first time, as building the extension currently fails in Node v18. [3] We're also updating the docs to mention using `nvm` to manage node versions. [1]: #1369 (comment) [2]: https://github.com/microsoft/vscode/blob/32d40cf44e893e87ac33ac4f08de1e5f7fe077fc/remote/.yarnrc#L2 [3]: #1373

As recommended here [1], since the current build for this extension does not work with Node v18 [2], it would be good to set a maximum node version until this gets addressed. So we're updating `engines` here to allow for a maximum version, which in this case is v17.0.0. [1]: #1369 (comment) [2]: #1373

As recommended here [1], we want to stay in sync with the current node version shipped with VSCode (16.13.0) [2]. For this we can add a `.nvmrc` file to alert nvm to switch to the preferred version automatically. It will also help prevent builds from failing when setting up the project for the first time, as building the extension currently fails in Node v18. [3] We're also updating the docs to mention using `nvm` to manage node versions. [1]: #1369 (comment) [2]: https://github.com/microsoft/vscode/blob/32d40cf44e893e87ac33ac4f08de1e5f7fe077fc/remote/.yarnrc#L2 [3]: #1373

As recommended here [1], since the current build for this extension does not work with Node v18 [2], it would be good to set a maximum node version until this gets addressed. So we're updating `engines` here to allow for a maximum version, which in this case is v17.0.0. [1]: #1369 (comment) [2]: #1373

elenatanasoiu · 2022-06-01T13:13:14Z

Thanks for the suggestions @aeisenberg, I've opened an issue here for Node v18 and a PR to update the engine field and add a .nvmrc file here.

As recommended here #1369 (comment), we want to stay in sync with the current node version [shipped with VSCode](https://github.com/microsoft/vscode/blob/32d40cf44e893e87ac33ac4f08de1e5f7fe077fc/remote/.yarnrc#L2) (v16.13.0). For this we can add a `.nvmrc` file to alert nvm to switch to the preferred version automatically. It will also help prevent builds from failing when setting up the project for the first time, as building the extension currently fails in Node v18: #1373 We're also updating the docs to mention using `nvm` to manage node versions.