Conversation
pisv
left a comment
There was a problem hiding this comment.
@ndoschek Thank you very much for all this effort! 👍
I can confirm that #16753 is fixed by this PR 🎉
I also tested git support to some extent and found a regression in the code editor's dirty diff peek view, where the actions for staging/reverting the current change were previously visible in the peek view's action bar like this:
but are not present there now:
(It is explicitly mentioned in the PR description regarding git support that all features might currently not be available, so a follow-up PR could probably fix this regression.)
|
Thanks @pisv for testing and reviewing, much appreciated! 🎉 |
|
|
||
| @inject(AIChatTreeInputConfiguration) @optional() | ||
| protected override readonly configuration: AIChatTreeInputConfiguration | undefined; | ||
| protected declare readonly configuration: AIChatTreeInputConfiguration | undefined; |
There was a problem hiding this comment.
You mention the switch from override to declare as an adjustment for the new TS version. What is the significance of the change, and are there particular conditions under which it's required? I tried reverting a few new declares to override, and the application still built, so at least in some cases, it doesn't seem that the new TS objects to the older override style. Is it a future-facing change to prepare for switch to the define semantics?
The declare keyword seems a little out of place, because for injections, at least, we are in fact setting the value, not just declaring its presence for type purposes. My interpretation of the keyword may be a little narrow, but the new usage feels at least unaccustomed.
There was a problem hiding this comment.
Thanks for the insights, that makes sense 👍
I double-checked as well and reverted the declare usages again. It looks like I made that change before enabling the relevant TS flag and didn't question it again afterwards. Given that the code still builds fine with override and there’s no clear benefit from switching in this case, I agree it’s better to keep the original approach and avoid unnecessary code changes.
| get onDidChangeReadOnly(): Event<boolean | MarkdownString> { | ||
| return this.document.onDidChangeReadOnly; | ||
| } | ||
|
|
||
| get onEncodingChanged(): Event<string> { | ||
| return this.document.onDidChangeEncoding; | ||
| } |
There was a problem hiding this comment.
There are a number of changes like this, of constructor-initialized field access -> getter. I imagine that falls under the heading of changes 'not to access fields before initialization.' I also tried reverting some of these and ran into no build or runtime problems. Is this also something that would be a problem if we switched to define-property semantics?
The reason I mention this is that if there's no build failure (and no lint rule) that detects these, new code very well may just bring in new instances, so if it's a problem, or something we want to make sure avoids becoming a problem, we may need some alternate means.
There was a problem hiding this comment.
Good point, thanks for calling this out 👍
I also tried reverting some of these changes and couldn’t reproduce any build or runtime issues. Similar to the previous case, to keep the changes minimal, I've reverted them as well.
| const id = this._hoverCounter; | ||
| // Check if hover map has more than 10 elements and if yes, remove oldest from the map | ||
| if (this._hoverMap.size === HoverAdapter.HOVER_MAP_MAX_SIZE) { | ||
| const minimumId = Math.min(...this._hoverMap.keys()); | ||
| this._hoverMap.delete(minimumId); | ||
| } | ||
| this._hoverMap.set(id, value); | ||
| this._hoverCounter += 1; |
There was a problem hiding this comment.
Probably better if the use of the ID and the incrementation of the counter are closer together so that no future code can insert control flow between them and create tricky conditional bugs.
| export function fromHover(hover: theia.VerboseHover): model.Hover { | ||
| return <model.Hover>{ | ||
| range: fromRange(hover.range), | ||
| contents: fromManyMarkdown(hover.contents) | ||
| contents: fromManyMarkdown(hover.contents), | ||
| canIncreaseVerbosity: hover.canIncreaseVerbosity, | ||
| canDecreaseVerbosity: hover.canDecreaseVerbosity, |
There was a problem hiding this comment.
Minor, old code: there doesn't appear to be any need for the cast here. Probably true in a number of places in this file.
There was a problem hiding this comment.
Agreed, I changed that cast here, I've created a task for myself to have a look at the entire type converter file at a later point.
| protected readonly selectionEmitter: Emitter<void> = new Emitter<void>(); | ||
| readonly onSelectionChange = this.selectionEmitter.event; | ||
| override id: string; | ||
| declare id: string; |
There was a problem hiding this comment.
This one is just bad: without strict property initialization, I don't think TS is actually doing anything to help us enforce that descendants of this class actually have an ID, regardless of whether we use override or declare.
| value = value.trim(); | ||
| } | ||
| (data[property] as string) = value; | ||
| (data[property] as unknown as string) = value; |
There was a problem hiding this comment.
I think the real fix here is not the cast, but to create a type like this:
type ProblemDataStringKeys = keyof { [K in keyof ProblemData as string extends ProblemData[K] ? K : never]: unknown };
And then use that instead of keyof ProblemData in this method and appendProperty. Then we exclude ProblemLocationKind and the type of data[property] is correctly computed to be string | undefined, so we can safely assign a string to it.
Resolves GH-16279 - update built-in extensions to version 1.104.0 and consume tar.gz archive from GitHub releases instead of open-vsx for now - add support for decompressing vsix bundles from tar.gz archives - ensure download:plugins command exits with proper exit codes - update theiaPlugins versions (EditorConfig, ESLint, Jupyter) - remove vscode.microsoft-authentication from excluded IDs (as it is not a standalone built-ins anymore) - remove ms-vscode.references-view from excluded IDs (it is now vscode.references.view and works without issues) Contributed on behalf of STMicroelectronics
- update base tsconfig target to ES2023 as we support >= Node 20 Resolves GH-16400 Contributed on behalf of STMicroelectronics
TypeScript 5+ with target ES2022+ changes how class fields are initialized by using Object.defineProperty semantics that can alter initialization order relative to super() calls. Several parts of our codebase rely on the older (ES2019-style) behavior where field assignments occur after the super() call, so this flag preserves compatibility for now. We can revisit this later and enable the standard behavior (useDefineForClassFields=true) once all affected classes are refactored to be spec-compliant. Contributed on behalf of STMicroelectronics
Main areas of update: - v regex flag is only available when targeting 'esnext' or later (we still use commonjs) - fix reported ts errors, add missing overrides Contributed on behalf of STMicroelectronics
Resolves GH-16397 - add proposed API definitions for editorHoverVerbosityLevel to support verbose hover (e.g. for typescript uses this in most cases now) - update plugin HoverAdapter that provides/releases hover - Remark: increase/decrease level will most likely work then with updated monaco version (GH-16401) Contributed on behalf of STMicroelectronics
|
Thank you both for the detailed feedback, much appreciated!
I assume this is linked to the monaco update, I'll take a note for the monaco upgrade followup to double check this then as well! I've updated my commits accordingly. When you have a moment, could you please take another look? TIA! |
colin-grant-work
left a comment
There was a problem hiding this comment.
With the new changes, this LGTM.
|
+1 :-) Thanks! |
What it does
update VS Code built-in extensions and plugin downloaderchore: upgrade typescript to ~5.9.3fix: disable ES2022 class field semantics useDefineForClassFields=falsefix: update code for new typescript versionvscode: implement proposed.editorHoverVerbosityLevel APIResolves GH-16279
Resolves GH-16400
Resolves GH-16397
Fixes GH-16753
More details about the switch to an archive instead of the extension pack:
Open VSX changed its ownership behavior, and now you need to claim a namespace to be able to publish an extension under it.
Our extension pack was already in the eclipse-theia namespace, but all the bundled extensions were under the vscode namespace. Unsurprisingly, the vscode namespace is claimed by Microsoft.
Previously, this wasn’t an issue, and we were still able to publish the extensions under the vscode namespace.
Also, the extension pack is an Open VSX concept.
As an intermediate solution, we discussed in the last dev meeting that we’ll temporarily skip the Open VSX publishing step, package the VSIX ourselves, and consume it with less effort for now, since the last built-in update was over a year ago.
We decided to postpone the final decision on whether we want to continue publishing on Open VSX and how to resolve the namespace problem.
We also considered changing the publisher to eclipse-theia, but this might lead to unforeseen issues.
For example, if other extensions depend on the built-ins, we might need some kind of hacky workaround to intercept and override the publisher prefix somewhere in Theia.
And since they were always intended to be "built-in", there's probably not too much benefit in having them listed in the store.
How to test
npm run clean && git clean -xdf)npm run allnpm run download:pluginsTypescript language support
other language support (like json, css etc)
git support (scm view, but not support all features fully yet, see also follow ups)
Start a sample task to check the line matcher still works as expected
e.g. add a .vscode/tasks.json file
{ "version": "2.0.0", "tasks": [ { "label": "Lint", "type": "shell", "command": "npm run lint", "problemMatcher": ["$tsc"] } ] }Observe logs for new exceptions/errors/warnings
Follow-ups
vscode.githubandvscode.github-authentication(see also Authentication provider registration of VSCode based extensions not working #16713)useDefineForClassFields=falsetopicBreaking changes
Attribution
Contributed on behalf of STMicroelectronics
Review checklist
nlsservice (for details, please see the Internationalization/Localization section in the Coding Guidelines)Reminder for reviewers