Skip to content
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

Debt - remove WinJS.Promise#cancel usage #56137

Closed
19 of 26 tasks
jrieken opened this issue Aug 10, 2018 · 29 comments
Closed
19 of 26 tasks

Debt - remove WinJS.Promise#cancel usage #56137

jrieken opened this issue Aug 10, 2018 · 29 comments
Assignees
Labels
debt Code quality issues engineering VS Code - Build / issue tracking / etc.

Comments

@jrieken
Copy link
Member

jrieken commented Aug 10, 2018

There are still a few WinJS.Promise#cancel usages and we should remove those. Find detail about how and why here #53526.

Todos for WinJS.Promise#cancel

Todos for WinJS.Promise creation with a non-empty cancel-callback

  • src/vs/base/node/request.ts#67 -> CancellationToken for request logic #57955
  • src/vs/base/common/async.ts#63
  • src/vs/base/common/async.ts#191
  • src/vs/base/common/async.ts#361
  • src/vs/base/test/common/async.test.ts#94
  • src/vs/base/test/common/async.test.ts#116
  • src/vs/base/test/common/utils.ts#21 @jrieken
  • src/vs/editor/standalone/browser/colorizer.ts#58 @alexandrudima
  • src/vs/workbench/api/node/extHostSearch.fileIndex.ts#436 @roblourens
  • src/vs/workbench/api/node/extHostSearch.fileIndex.ts#482
  • src/vs/workbench/api/node/extHostSearch.fileIndex.ts#483
  • src/vs/workbench/api/node/extHostSearch.fileIndex.ts#506
  • src/vs/workbench/api/node/extHostSearch.fileIndex.ts#551
  • src/vs/workbench/api/node/extHostSearch.fileIndex.ts#632
  • src/vs/workbench/api/node/extHostSearch.fileIndex.ts#667
  • src/vs/workbench/api/node/extHostSearch.fileIndex.ts#694
  • src/vs/workbench/api/node/extHostSearch.ts#86 -> Use CancellationToken for search #57957
  • src/vs/workbench/api/node/extHostSearch.ts#311 -> Use CancellationToken for search #57957
  • src/vs/platform/request/electron-browser/requestService.ts#25 -> CancellationToken for request logic #57955
  • src/vs/base/parts/tree/browser/treeModel.ts#89 @joaomoreno
  • src/vs/base/parts/ipc/node/ipc.ts#313
  • src/vs/workbench/services/extensions/node/lazyPromise.ts#40
@jrieken jrieken added debt Code quality issues engineering VS Code - Build / issue tracking / etc. labels Aug 10, 2018
@jrieken jrieken added this to the September 2018 milestone Aug 10, 2018
@jrieken
Copy link
Member Author

jrieken commented Aug 10, 2018

The goal is to finish this in September but it won't hurt to remove one or two usages in August

joaomoreno added a commit that referenced this issue Aug 10, 2018
joaomoreno added a commit that referenced this issue Aug 10, 2018
joaomoreno added a commit that referenced this issue Aug 10, 2018
isidorn added a commit that referenced this issue Aug 15, 2018
@isidorn isidorn removed their assignment Aug 15, 2018
@bpasero
Copy link
Member

bpasero commented Aug 17, 2018

I opened #56656 for the zip.ts case.

@jrieken this is not just TPromise#cancel() but also every new TPromise(..., onCancel), did you account for that as well? There may be more clients of that pattern.

@jrieken
Copy link
Member Author

jrieken commented Aug 17, 2018

every new TPromise(..., onCancel), did you account for that as well?

Theory is that we can delete that once no more calls to cancel exist

@bpasero bpasero removed their assignment Aug 22, 2018
@sandy081 sandy081 removed their assignment Sep 3, 2018
@jrieken
Copy link
Member Author

jrieken commented Sep 3, 2018

@bpasero Assigned you back for getResults(searchValue: string): TPromise<IModel<any>> on which search cancellation depends.

@bpasero
Copy link
Member

bpasero commented Sep 3, 2018

There is now a CancellationToken that is passed on to the quick open handler which will be cancelled when quick open closes or changes. I need support for this token in the following places before I can remove the cancel() method in openAnythingHandler:

@roblourens
Copy link
Member

I still cancel promises normally across the IPC right? I'd left some of my references which are close to the IPC (but I'll remove it from the SearchService API)

@jrieken
Copy link
Member Author

jrieken commented Sep 6, 2018

@sandy081 sandy081 removed their assignment Sep 6, 2018
@sandy081
Copy link
Member

sandy081 commented Sep 6, 2018

@joaomoreno Added you for extensionEditor. Cancellation here needs to be travelled till gallery/request service.

@sandy081
Copy link
Member

sandy081 commented Sep 6, 2018

@joaomoreno One ✅ less for you.. I took care of handling cancellation token in Extension editor. Thanks to @jrieken's PR of adding cancellation token to request API.

FYI I did not add cancellation tokens for all APIs in Gallery service, I only added to those which are needed and can be added to others on need. Feedback is welcome on the changes.

@jrieken
Copy link
Member Author

jrieken commented Sep 7, 2018

We are now free of asWinJSPromise and free of wireCancellationToken! Thanks everyone so far.

@bpasero re #56137 (comment). I left a todo here which is about supporting cancelation in quick open handlers.

@bpasero bpasero removed their assignment Sep 8, 2018
joaomoreno added a commit that referenced this issue Sep 10, 2018
@joaomoreno
Copy link
Member

joaomoreno commented Sep 10, 2018

72484fd removes WinJS.Promise dependency from IPC. Its dependency is solely on Thenable objects. 🎆

@roblourens
Copy link
Member

🎉 DONE 🎉

@jrieken
Copy link
Member Author

jrieken commented Sep 11, 2018

We are done! There is no more WinJS.Promise#cancel. Thanks to everyone helping with this!

iu

@jrieken jrieken closed this as completed Sep 11, 2018
@joaomoreno
Copy link
Member

joaomoreno commented Sep 11, 2018

yay

@vscodebot vscodebot bot locked and limited conversation to collaborators Oct 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues engineering VS Code - Build / issue tracking / etc.
Projects
None yet
Development

No branches or pull requests

8 participants