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

Convert to ES6, Promises, async and await. #137

Merged
merged 1 commit into from
Feb 11, 2016

Conversation

johnfn
Copy link
Member

@johnfn johnfn commented Feb 8, 2016

Converts the entire project to use ES6 Typescript, and converts all promise based code (that I could find) to use async/await.

This makes all of the Promise-based code we had before look a lot nicer. Compare e.g. test/mode/modeInsert.test.ts, which used to be an awful mess of .then(), with how nice it looks now! Since a good chunk of the VSC API is promise-based, I think this will help beautify a lot of our code in the future.

There is a drawback to this approach, which is that until microsoft/vscode#2778 gets resolved, I had to disable some of the ES6 features we were using that are currently hidden behind node flags. These are:

  • Default parameters - function foo(x: number = 6)
  • Rest arguments - function foo(...x: number[])
  • The default export - export default function foo()

I don't see these things as a huge loss compared to the gain of async/await. The frustrating thing is that if you do e.g. use default arguments, you get a fairly obscure error message during runtime that, if you've never seen it before, could be pretty confusing. If the VSC team gets back to us quickly on microsoft/vscode#2778 it won't be a big issue, but if not we could catch it by wrapping import statements in a try catch and giving a more helpful error message.


I also took the opportunity to make a few minor stylistic changes:

  • For modes, we were naming functions in TitleCase, which was inconsistent with the rest of our naming conventions. I changed it to camelCase.
  • Use for ... of in some places. for ... of is objectively better than for (let i = 0; i < arr.length; i++) { let obj = arr[i]... and subjectively better than _.forEach because it is prettier. 😄

Hopefully the above isn't contentious and I can sneak it in this PR 😉

@jpoon
Copy link
Member

jpoon commented Feb 9, 2016

Yay! Thank you. I've been always meaning to do this! Although, looks to be a merge conflict, likely caused I just merged #138

@johnfn
Copy link
Member Author

johnfn commented Feb 9, 2016

@jpoon 👍 Just fixed up the merge conflicts, should be good to go!

@jpoon
Copy link
Member

jpoon commented Feb 10, 2016

@johnfn dammit. I should really merge this PR first. Conflicts again after merging your other one :(

@johnfn
Copy link
Member Author

johnfn commented Feb 11, 2016

@jpoon Totally unacceptable.

@johnfn
Copy link
Member Author

johnfn commented Feb 11, 2016

...But I fixed it up anyways. 👍

jpoon added a commit that referenced this pull request Feb 11, 2016
Convert to ES6, Promises, async and await.
@jpoon jpoon merged commit f5a6625 into VSCodeVim:master Feb 11, 2016
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants