Skip to content

Ensure that TS can report todo comments even prior to us having connected to the OOP server.#43410

Merged
CyrusNajmabadi merged 2 commits intodotnet:masterfrom
CyrusNajmabadi:asyncReport
Apr 23, 2020
Merged

Ensure that TS can report todo comments even prior to us having connected to the OOP server.#43410
CyrusNajmabadi merged 2 commits intodotnet:masterfrom
CyrusNajmabadi:asyncReport

Conversation

@CyrusNajmabadi
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi commented Apr 16, 2020

Fixes #43320 (comment)

TS is going to be reporting these comments using their own mechanism for scheduling. Technically, nothing ensures that they will be runnign after we "start" (i.e. hook up to OOP) our TODO service. This is esp. the case since we launch this in a Fire-And-Forget manner in async-package-load.

This PR just makes us resilient to the TS calling into us before that. In that case, all we do is just have their call await us actually getting started and then from that point on everything is hooked up properly and messages are processed normally.

@CyrusNajmabadi CyrusNajmabadi requested review from amcasey and tmat April 16, 2020 18:58
@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner April 16, 2020 18:58
@rachelgshaffer
Copy link

I exist!

@CyrusNajmabadi
Copy link
Contributor Author

@tmat @jasonmalinowski PTAL. This is for partner team to help with the pain i caused over moving Todo OOP.

Copy link
Member

@tmat tmat left a comment

Choose a reason for hiding this comment

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

:shipit:

@CyrusNajmabadi
Copy link
Contributor Author

Wrking to get confirmation from TS this solves their problem. Once we know for certain, we can merge.

@amcasey
Copy link
Member

amcasey commented Apr 16, 2020

I don't fully understand this work queue implementation, but the idea seems sensible. @rachelgshaffer would be the one to confirm practical correctness for TS. 👍

@amcasey
Copy link
Member

amcasey commented Apr 16, 2020

While you're touching this file, you might want to clean up the TS-specific method at the bottom - it will never find a tree for the document.

@CyrusNajmabadi
Copy link
Contributor Author

While you're touching this file, you might want to clean up the TS-specific method at the bottom - it will never find a tree for the document.

Good point :)

@rachelgshaffer
Copy link

I was able to debug and verify this works from the TS side. thanks for your help!

@CyrusNajmabadi CyrusNajmabadi changed the title Ensure that TS can report todo comments even prior to us having coonnected to the OOP server. Ensure that TS can report todo comments even prior to us having connected to the OOP server. Apr 23, 2020
@CyrusNajmabadi CyrusNajmabadi merged commit 79b68aa into dotnet:master Apr 23, 2020
@ghost ghost added this to the Next milestone Apr 23, 2020
@CyrusNajmabadi CyrusNajmabadi deleted the asyncReport branch April 23, 2020 20:57
@sharwell sharwell modified the milestones: Next, temp, 16.7.P1 Apr 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Workspace change events fire before IVsTypeScriptTodoCommentService is prepared to receive calls to ReportTodoComments

7 participants