-
Notifications
You must be signed in to change notification settings - Fork 293
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
Fix hover regression in webview editor #4299
Conversation
@@ -339,20 +339,6 @@ jobs: | |||
- name: Install dependencies (npm ci) | |||
run: npm ci --prefer-offline | |||
|
|||
- name: Install Dot.net |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't necessary for functional tests and was intermittently failing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added them with the intention of keeping all actions consistently the same...
Else it was getting too difficult to figure out what was reqquired
hate this copy paste nonense...
@@ -262,14 +262,16 @@ function convertToMonacoMarkdown( | |||
| vscode.MarkedString | |||
| vscode.MarkedString[] | |||
): monacoEditor.IMarkdownString[] { | |||
if (strings.hasOwnProperty('kind')) { | |||
// tslint:disable-next-line: no-any | |||
if (strings.hasOwnProperty('kind') || (strings as any).kind) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new condition is sufficient (strings as any).kind
, won't need hasOwnProperty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not true as (strings as any).kind will resolve as false if the value is 0 or false. I want it to continue to work if the property happens to be there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't 'in' keyword work here? 'kind' in strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me double check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that seems to work. Not sure what the difference is. I assumed 'in' used hasOwnProperty under the covers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rchiodo Heh, I only know because I'd been going through a JS basics book (felt like there was stuff that I didn't know jumping directly into TS) and just read the chapter covering JS object. hasOwnProperty is only checking the top level object prototype for the property, so if it's inherited it won't show up. 'in' keywork checkes the entire property chain. So for an object hasOwnProperty('toString') return false, since that method is in the base object prototype. kind must not be on the top object for some of those string types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might as well remove the check for hasOwnProperty
Codecov Report
@@ Coverage Diff @@
## main #4299 +/- ##
=======================================
+ Coverage 70% 75% +4%
=======================================
Files 389 389
Lines 25445 25456 +11
Branches 3637 3639 +2
=======================================
+ Hits 18046 19191 +1145
+ Misses 5996 4781 -1215
- Partials 1403 1484 +81
|
For #4218
Root cause was the structure returned by the language server did not show 'hasOwnProperty' as true even though the field was present.
package-lock.json
has been regenerated by runningnpm install
(if dependencies have changed).