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 all Coffeescript to ES6 #2884

Open
jackiehluo opened this issue Oct 1, 2016 · 8 comments

Comments

Projects
None yet
6 participants
@jackiehluo
Copy link
Contributor

commented Oct 1, 2016

to the ☕ script! ES6 is cleaner, more modern, and has a much more reliable linter—plus, people know how to read it—so we want to convert everything in the codebase. Most of the code is already there, but there are still specs written in Coffee.

@krngrvr09

This comment has been minimized.

Copy link

commented Oct 1, 2016

Hello, I would like to work on this issue :)

@zcserei

This comment has been minimized.

Copy link

commented Oct 1, 2016

I would also like to contribute!

@krngrvr09

This comment has been minimized.

Copy link

commented Oct 1, 2016

Wow! There are 265 .coffee files. This will take some time I guess. But I agree its a good starter issue.

@jonboiser

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2016

Hi everyone, I just wanted to share what I did for the small PR I just submitted.

Steps

  1. Followed the contribution guide to get the app installed.
  2. Ran the bootstrap script and test script.
  3. In Atom, I was getting errors trying to use the local eslint setup, so had to install eslint, babel-eslint, and airbnb to my global npm. Not sure what is up with that.
  4. Used decaffeinate to quickly convert the coffee file.
  5. renamed that output to use the es6 file extension
  6. cleaned up the es6 file
  7. ran the ./N1.sh --test --enable-logging commands a few times, changing the spec as a sanity check.

This particular spec was kind of weird, since it doesn't have an assertion, so I forced it to fail by adding expect(true).toBe(false) at the end as a sanity check.

@jackiehluo

This comment has been minimized.

Copy link
Contributor Author

commented Oct 1, 2016

Yay, thanks so much, all! Just FYI, particularly for these Coffeescript ➡️ ES6 conversions, you can also run the linter with script/grunt lint. We've had some trouble with decaffeinate in the past (since it's not always perfect), so it's good to do a sanity check by making sure the code is still working the same way.

@acupoftee

This comment has been minimized.

Copy link

commented Oct 20, 2016

Is it too late to contribute to this? I would definitely like to contribute to this!

@jackiehluo

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2016

@BethyDiakabananas Not at all! You should!

@oksas

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2017

@jonboiser, thanks for the steps on conversion; it was super helpful in getting me started quickly. I am about to submit a PR for another small, single file conversion and thought it worth noting a few other things:

  • If you use decaffeinate to convert initially, you'll probably end up having to convert a lot of let into const for the linter's sake, so it might be worth running decaffeinate with the --prefer-const flag (ie decaffeinate some-file-spec.coffee --prefer-const)
  • Again if you use decaffeinate, it seems to put a lot of return statements at the end of blocks that don't belong and you can safely remove. I'm guessing CoffeeScript has implicit last line returns or something. So after the initial conversion I ended up with a lot of things like return expect(somethingsomething)... at the end of it blocks, and even the final it block within a describe block was getting a return placed before it too, etc.
  • In the case of spec files at least, if anything gets put on this in beforeEach or something similar, and then subsequently used in tests, you'll have to ensure that the spec's outermost describe callback function is not an arrow function, otherwise this bindings won't be what is expected and the tests will fail. For example, the top of a test file I just converted:
...

describe("ActionBridge", function actionBridge() {
  describe("in the work window", () => {
    beforeEach(() => {
      spyOn(NylasEnv, "getWindowType").andReturn("default");
      spyOn(NylasEnv, "isWorkWindow").andReturn(true);
      this.bridge = new ActionBridge(ipc);
    });

...

Note that the outermost callback function is a regular, named function expression and not an arrow function, but all other callbacks in that file are just arrow functions and it runs fine 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.