Skip to content

Comments

Update tsc to 4.5.5#1202

Merged
aeisenberg merged 1 commit intomainfrom
aeisenberg/update-tsc
Mar 25, 2022
Merged

Update tsc to 4.5.5#1202
aeisenberg merged 1 commit intomainfrom
aeisenberg/update-tsc

Conversation

@aeisenberg
Copy link
Contributor

The default version of tsc in vscode is now 4.5.4. This version
has changed the type of the variable in the catch block.
Previously, it was any. Now it is unknown.

This change updates vscode so that it can build with 4.5.4.

Previously, this had been a bit of a pain since sometimes running
a compile task in vscode will use the global default version of
tsc.

This is also a chance to try out the new dependency review workflow.

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 aeisenberg requested review from a team as code owners March 11, 2022 20:49
return dot;
} catch (err) {
throw new Error(`Reading output of interpretation failed: ${err.stderr || err}`);
throw new Error(`Reading output of interpretation failed: ${getErrorMessage(err)}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to give priority to err.stderr here as well?

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 think that was an error. There is no stderr property on the Error class.

Copy link
Contributor

Choose a reason for hiding this comment

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

There were other places in this file where err.stderr || err is translated to
(err as any).stderr || getErrorMessage(err). Is the difference due to the catch blocks catching different objects? If so, it might be useful to document how we can tell which expression to use where, for the next person who needs to modify this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's interesting. I am really struggling to find out why err.stderr is being used, or if that property even exists.

I suspect that it is coming from here:

const result = await promisify(child_process.execFile)(codeQlPath, args);

await promisify(child_process.execFile)(...);

returns an object with { stderr, stdout }. It may be that when catching the error from a child process, the stderr property is being read (even though that's a mistake and it doesn't exist). The other locations in the code could just be copies of the first.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you have a better handle on what is happening here than I do, so I will let you do what you think is best.

@aeisenberg aeisenberg force-pushed the aeisenberg/update-tsc branch from 50d6d0f to a55a7e7 Compare March 25, 2022 16:40
The default version of tsc in vscode is now 4.5.4. This version
has changed the type of the variable in the catch block.
Previously, it was `any`. Now it is `unknown`.

This change updates vscode so that it can build with 4.5.4.

Previously, this had been a bit of a pain since sometimes running
a compile task in vscode will use the global default version of
tsc.
@aeisenberg aeisenberg force-pushed the aeisenberg/update-tsc branch from a55a7e7 to 23e29a1 Compare March 25, 2022 16:49
@aeisenberg aeisenberg merged commit 0b9fcb8 into main Mar 25, 2022
@aeisenberg aeisenberg deleted the aeisenberg/update-tsc branch March 25, 2022 18:33
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.

2 participants