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

Instantiate native promise instead of WinJS.Promise #62716

Closed
1 task
jrieken opened this issue Nov 7, 2018 · 12 comments
Closed
1 task

Instantiate native promise instead of WinJS.Promise #62716

jrieken opened this issue Nov 7, 2018 · 12 comments
Assignees
Labels
debt Code quality issues engineering VS Code - Build / issue tracking / etc.
Milestone

Comments

@jrieken
Copy link
Member

jrieken commented Nov 7, 2018

This is part of #53526 and points out files in which we call new WinJS.Promise. In addition to join, as, wrap, and wrapError that's how winjs promises end up in our system and we wanna get rid of them.

Please migrate to instantiating native promises with taking all care that's needed for this adoption.

@bpasero

  • src/vs/platform/lifecycle/electron-main/lifecycleMain.ts
@jrieken jrieken added the debt Code quality issues label Nov 7, 2018
@jrieken jrieken added this to the November 2018 milestone Nov 7, 2018
@jrieken jrieken added the engineering VS Code - Build / issue tracking / etc. label Nov 7, 2018
@dbaeumer
Copy link
Member

dbaeumer commented Nov 7, 2018

Started this but need to wait until lower level stuff like src/vs/workbench/services/files/electron-browser/fileService.ts is converted.

@dbaeumer
Copy link
Member

dbaeumer commented Nov 7, 2018

Did:

src/vs/base/node/processes.ts
src/vs/workbench/parts/tasks/common/problemMatcher.ts
src/vs/workbench/parts/tasks/common/taskDefinitionRegistry.ts

@alexr00 can you do src/vs/workbench/parts/tasks/electron-browser/terminalTaskSystem.ts that I don't interfere with work you are doing.

@roblourens roblourens removed their assignment Nov 7, 2018
@roblourens
Copy link
Member

Using Thenable helps when the file's dependencies aren't converted. e.g. in cases like

public foo(): Thenable<void> {
  return actuallyTPromise().then(() => something)
}

As long as you check that the code will be safe when the upstream method switches from TPromise to Promise.

@jrieken
Copy link
Member Author

jrieken commented Nov 8, 2018

Yes, there are three ways to deal with not-yet-converted dependencies (and I think that 1 and 2 are the best ways to tackle this)

  • use Thenable
  • or use Promise.resolve
  • use async-await

@dbaeumer
Copy link
Member

dbaeumer commented Nov 8, 2018

Makes sense. Since the fileService is on the list to be converted I will wait. Will ensure I only need to touch once.

@chrmarti chrmarti removed their assignment Nov 8, 2018
@alexdima alexdima removed their assignment Nov 10, 2018
joaomoreno added a commit that referenced this issue Nov 15, 2018
joaomoreno added a commit that referenced this issue Nov 15, 2018
joaomoreno added a commit that referenced this issue Nov 16, 2018
joaomoreno added a commit that referenced this issue Nov 16, 2018
joaomoreno added a commit that referenced this issue Nov 16, 2018
@joaomoreno joaomoreno changed the title instantiate native promise instead of WinJS.Promise Instantiate native promise instead of WinJS.Promise Nov 19, 2018
joaomoreno added a commit that referenced this issue Nov 19, 2018
joaomoreno added a commit that referenced this issue Nov 19, 2018
@joaomoreno
Copy link
Member

Completed my set:

@joaomoreno

src/vs/base/common/async.ts
src/vs/base/common/paging.ts
src/vs/base/parts/tree/browser/treeModel.ts
src/vs/base/parts/tree/test/browser/treeModel.test.ts
src/vs/code/electron-browser/sharedProcess/sharedProcessMain.ts
src/vs/code/electron-main/sharedProcess.ts
src/vs/base/test/common/paging.test.ts
src/vs/platform/driver/electron-main/driver.ts
src/vs/platform/update/electron-main/updateService.win32.ts
src/vs/code/node/cli.ts

Words of warning for @Microsoft/vscode: since this touched both the old tree as well as async.ts, be on the lookout for bugs. Remember this issue.

@jrieken
Copy link
Member Author

jrieken commented Nov 22, 2018

ping @bpasero and @Tyriar

@chrmarti chrmarti removed their assignment Nov 23, 2018
@alexdima
Copy link
Member

@bpasero Why the reassign? Are there more files I should look at?

I've done the files under my name.

@alexdima alexdima removed their assignment Nov 23, 2018
@bpasero
Copy link
Member

bpasero commented Nov 23, 2018

@alexandrudima sorry I thought people would remove the ones that are done from the summary like Joao did.

@bpasero bpasero closed this as completed Nov 26, 2018
bpasero added a commit that referenced this issue Nov 26, 2018
* remove more TPromise (for #53526)

* fix failing webview integration tests on macOS

* do not instantiate TPromise (fix #62716)

* more TPromise removal and fixes

* 💄

* 💄
@jrieken jrieken reopened this Nov 26, 2018
@bpasero bpasero modified the milestones: November 2018, December 2018 Nov 26, 2018
@bpasero
Copy link
Member

bpasero commented Nov 26, 2018

Had to revert the one in lifecycleMain.ts, that one is a bit tricky, will revisit in December. But that is really the last one 🔨

@bpasero
Copy link
Member

bpasero commented Nov 26, 2018

Ah whatever, converted the last one 💯

@bpasero bpasero closed this as completed Nov 26, 2018
@bpasero bpasero modified the milestones: December 2018, November 2018 Nov 26, 2018
bpasero added a commit that referenced this issue Nov 26, 2018
@jrieken
Copy link
Member Author

jrieken commented Nov 26, 2018

awesome!

@vscodebot vscodebot bot locked and limited conversation to collaborators Jan 10, 2019
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

9 participants