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

Migrate codebase from coffeescript to ES6 #3459

Closed
philc opened this issue Jan 5, 2020 · 7 comments
Closed

Migrate codebase from coffeescript to ES6 #3459

philc opened this issue Jan 5, 2020 · 7 comments
Labels

Comments

@philc
Copy link
Owner

philc commented Jan 5, 2020

Hey folks,

I plan to modernize the code base by porting it from coffeescript to ES6. This will make the source more accessible to more developers, and remove the complexity of having to transpile.

Major impacts of this:

  1. Some hard-to-notice bugs may be introduced by the decaffeinate conversion. When I've used decaffeinate on large code bases before, I've botched a few cases of removing unnecessary implicit return statements which are on by default on coffeescript.

  2. All of the open PRs will become unmergeable and require revision before they can be re-evaluated.

  3. Forks of Vimium will no longer be able to cleanly rebase on top of our recent commits.

  4. It will become harder to find the history behind each line using git blame and git grep.

I wanted to give a heads up that this is coming, in case anyone has strong objections. The tentative date for the conversion is Jan 20, 2020. I may ship a new release to the store before then, with a few bug fixes.

Regarding the open PRs, I'm going to review the most recent 20-30 and merge those which seem valuable and low-risk. If a change is large and has a strong risk of destabilizing the code base, I'll likely not merge it and ask that it be reworked post the ES6 conversion. In case anyone wants to advocate strongly for an existing open PR to get merged before the conversion, now is the time.

Once the source is converted to ES6, I'd appreciate beta testers using the version on master, to help surface breakages in features that I don't normally use, before this version gets shipped to the chrome store to a wider audience.

Thanks! This should be great!

@philc
Copy link
Owner Author

philc commented Feb 9, 2020

This has begun in 791d737.

@imbrn
Copy link

imbrn commented Feb 11, 2020

What about Typescript? 🤓

@philc
Copy link
Owner Author

philc commented Feb 16, 2020

Formal types would be nice, but I want to generally avoid transpilation, to keep the development model simple.

@gdh1995
Copy link
Contributor

gdh1995 commented Feb 16, 2020

Just a small tip: async / await are in ES2017 and only exists on Chrome 57+, so if we remove transpilation completely, then new developers have to take care of not using features newer than ES2015, like spread properties (...).

@philc
Copy link
Owner Author

philc commented May 20, 2020

@gdh1995 Great, thanks for calling this out. I'll bump the minimum required versions of Firefox and Chrome so we can use ES2017, and note which version of javascript developers are allowed to use in CONTRIBUTING.md.

Ok, this conversion is now complete. I did the port very carefully using decaffeinate with a lot of subsequent tuning of the input coffeescript, and then the converted javascript, but even still, there are at least 3 bugs I was able to easily spot after some usage, which I didn't spot while I was in the middle of the conversion: #3566, #3567, #3568. I expect there are many more small ones which will surface when others start using this (I don't regularly use all of the features of Vimium). Please file them when you see them.

Retrospective: I'm very thankful to be done with Coffeescript. It has persisted long past its usefulness. Besides it being a barrier to entry for many javascript developers, it added insidious complexity which was gnawing at our code base:

  1. Its syntactic sugar introduced a lot of ambiguity in the code that that had to be resolved as part of the javascript conversion. E.g. "which of these implicit return statements are really necessary?". I left many in the code base because eliminating every unnecessary statement was too time consuming, given our incomplete unit test coverage.
  2. Required transpilation, which necessitates a longer dev tool chain, and makes it a two step process to debug exceptions thrown by our generated javascript when running in Chrome.
  3. The generated javascript code was pretty ugly, partly because of the implicit returns.

From this point forward, I intend to make the code base increasingly simple. This was the first big step.

@philc philc closed this as completed May 20, 2020
@philc philc added the dev-ux label May 20, 2020
philc added a commit that referenced this issue May 20, 2020
Since the removal of coffeescript in #3459, we're now using some newer
features of javascript. I think the most recent feature we're currently using is
Array.flat, which was introduced in Chrome 69 and Firefox 62.

I checked the google analytics for our chrome store page, and less than
0.5% of users visiting that page have a Chrome version prior to 80, so
this increased version requirement should be acceptable.

We don't currently enforce a minimum version for Firefox; I'll do that
in a separate commit.
@philc
Copy link
Owner Author

philc commented May 20, 2020

@gdh1995 I've bumped the minimum versions for Chrome and Firefox to ensure support for ES2018 in b243ab6 and fbddc22.

@gdh1995
Copy link
Contributor

gdh1995 commented May 20, 2020

checked google analytics for our chrome store page

How did you do that ? I'm also caring about the statistics of Chrome versions, but the extension dashboard doesn't provide it.

have a Chrome version prior to 80

Maybe those old users don't need to visit Chrome Store page. And they may stay on an old Chrome.

Anyway, according to https://caniuse.com/usage-table , Chrome 69 only owns 0.09% of the total market , so 69 seems OKay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants