-
Notifications
You must be signed in to change notification settings - Fork 19
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
JavaScript #4
base: master
Are you sure you want to change the base?
JavaScript #4
Changes from 3 commits
6dcd2f4
7d3b514
fbc1e10
cf16911
93ada84
6bbe7a5
3d3fd6a
d499b70
7ce6add
5575a5f
f87f365
853491d
1008fa7
1d91ad8
cbd1fc2
837bd2f
0556920
df32778
b0655b9
a400b15
a8dee2d
3d3c579
10e2a51
d2d3938
6fbb8af
114937e
908a971
f693a59
6d19dcd
824481b
b21b7a0
f6cc3ba
be1550c
fc03570
8a77fec
ebf1bfe
f1a9cfe
25e0c30
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,110 @@ | ||
--- | ||
state: In Progress | ||
--- | ||
|
||
# JavaScript | ||
|
||
## Motivation | ||
|
||
This evolution proposal is part of point 3 of the Hubot 3.0 milestone: | ||
|
||
> Modernize the project by translating CoffeeScript to JavaScript, improving integration with various developer tools, and adding features that make it easier for developers to automate their workflow. | ||
|
||
**Scope:** | ||
|
||
- Simplify contribution / maintenance | ||
- Removal of tooling / build steps | ||
- Align Node support with [Node’s LTS schedule](https://github.com/nodejs/LTS#lts-schedule1) | ||
- maintain backwards compatibility of user scripts written in CoffeeScript | ||
|
||
**Out of scope:** | ||
|
||
- no new features or breaking changes (beyond dropping support for Node <v4) | ||
- Introduce any other tooling besides linter | ||
- Any non-trivial refactoring of the existing code base | ||
|
||
## Proposed solution | ||
|
||
Converting of all CoffeeScript to JavaScript which can run natively on Node 4 ([current maintenance version until April 2018](https://github.com/nodejs/LTS#lts-schedule1)). | ||
|
||
The only tool I would like to introduce is a linting tool which will be run as part of `npm test`. | ||
|
||
I suggest [standard](https://standardjs.com/). We’ve been using it in all our projects at [Hoodie](http://hood.ie/) and [Neighbourhoodie](http://neighbourhood.ie) since 2+ years and never looked back. It's a zero-configuration JavaScript linter. | ||
|
||
## Detailed process | ||
|
||
**TODO:** make one or two sample CoffeeScript to JavaScript conversions (I’m working on it right now and will make it part of the proposal PR for the evolution repository) | ||
|
||
The steps of the conversation will be | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the intent to do each of these as a PR per item? One big PRs? Some combination? |
||
|
||
- [ ] Convert source files from CoffeeScript to JavaScript. | ||
- [ ] Convert test files from CoffeeScript to JavaScript. | ||
- [ ] update package.json | ||
- [ ] add script for JavaScript linting | ||
- [ ] Update documentation | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Everything in hubotio/hubot#1327 should also be in this list, especially https://github.com/github/generator-hubot There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The documentation should be updated before the release. I want to make sure that the files that are packaged include updated documentation with how to use the Javascript support. |
||
|
||
### Convert source files from CoffeeScript to JavaScript | ||
|
||
Convert all source files from CoffeeScript to JavaScript with a tool like [espresso](https://github.com/HipsterBrown/espresso). | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The author of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good timing, I just found out about it and am looking into it right now. It looks like the better option for us. I’ll keep you posted |
||
Make sure the existing tests (still written in CoffeeScript) run against the new JavaScript. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My previous hubot -> ES6 porting experience indicate |
||
|
||
Now go trough each file and improve the code readability by hand as needed. From my experiences with converting Hoodie from CoffeeScript to JavaScript quite a lot of manual work will be required. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/trough/through/ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be clear, this is improving internal readability, and is leaving the external API in-tact? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. correct |
||
|
||
### Convert test files from CoffeeScript to JavaScript | ||
|
||
Same as the previous steps only for the tests files this time. | ||
|
||
When finished, create a separate branch with with the tests written in JavaScript but the source code still in CoffeeScript to assure integrity. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 😍 This sounds like a great way to build confidence in the conversion. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a |
||
|
||
### Update package.json | ||
|
||
Remove all CoffeeScript related dependencies and tooling. Update [engines](https://docs.npmjs.com/files/package.json#engines) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do existing bots have their own coffeescript dependency, or do they rely on hubot's? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would make sense to begin to build some script to make an inventory, like we had with https://github.com/hubotio/hubot-scripts-list so we can have data on existing population of plugins. I think I will give that a try, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This step still needs to be done. I see There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. startet PR for engines: We still have the Is there a better way to achieve the same? |
||
|
||
Make sure that Hubot scripts can still be written in CoffeeScript. We want to break as few existing scripts as possible and many of them are written in CoffeeScript. We treat it as legacy support and will drop support for scripts written in CoffeeScript in future. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How can we make sure coffeescript tests still work? Maybe something like a test hubot instance for automated testing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea, I thought of testing it manually, but will try to add an automated e2e test for CI |
||
|
||
### Add script for JavaScript linting | ||
|
||
The only tool I would like to introduce as part of this proposal is a linting tool which will be run as part of `npm test`. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we make sure to update (or create?) contributor documentation around using the linter? I've had times where I've started on new process, with CI running linter, and the feedback process is relatively slow until you actually start getting it running locally, if not as part of your editor setup. |
||
|
||
I suggest [standard](https://standardjs.com/). We’ve been using it in all our projects at Hoodie and Neighbourhoodie since 2+ years and never looked back. It's a zero-configuration JavaScript linter. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 from me for standard. I've only used it a couple times, but I like that it is zero config, and the style it encourages is pretty light on line-noise (ie no semi-colons). |
||
|
||
### Update documentation | ||
|
||
Remove any reference of CoffeeScript from the documentation. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it's considered legacy, maybe we should still have documentation until it's removed? Even if it's removed, it might be worth having some way to get to the old documentation (ie a link in the readme to an older sha?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel that we need to duplicate the documentation, keep the old one somewhere. Because many people, for legitimate reason, will wait hubot3 stability before jumping in. |
||
|
||
## Backward compatibility | ||
|
||
Only breaking change is dropping support of Node 0.10 and 0.12. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The way that hubot is launched could be a possible breakage:
It should be noted it's plausible that code in the wild calls |
||
|
||
## Alternatives considered | ||
|
||
- **[CoffeeScript 6.0](https://github.com/coffeescript6/discuss)** | ||
|
||
As stated in the [Background section](https://github.com/coffeescript6/discuss#background), JavaScript caught up with most of the features that only CoffeeScript offered in the past. | ||
|
||
The argument for the clear syntax is valid. But we can enforce that with a JavaScript linter. | ||
|
||
The main reason we decided to convert Hubot to JavaScript is to make it easier to contribute to for a wider range of people. | ||
|
||
- **Continue to support Node <v4** | ||
|
||
Node versions smaller than v4 are no longer maintained and have known security vulnerabilities. The only major operating system which distributes a no longer maintained Node version by default is Debian, and there are alternative ways to install a recent Node version, see [Debian and Ubuntu based Linux distributions](https://nodejs.org/en/download/package-manager/#debian-and-ubuntu-based-linux-distributions). | ||
|
||
Mikeal Rogers (Node Foundation) says about Debian’s official Node packages: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a relevant link for this, ie tweet, documentation, etc? |
||
|
||
> The "official" repositories are outdated and even insecure. We need to treat them as legacy and move on. | ||
|
||
The Node Foundation has tried to get Debian to adapt modern Node versions like "[pretty much every other repository for packages takes modern versions.](https://twitter.com/mikeal/status/869646796888330240)" but could not succeed. | ||
|
||
See more responses to my question regarding old Node versions at https://twitter.com/gr2m/status/869305267464306689 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Debian users that need modern node version have 2 main easy options:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good point, we could document these options as part of our breaking changes documentation / changelogs |
||
|
||
- **Dropping support for Node <v7.6** | ||
|
||
_**Note:** this is still up for discussion even though the text below is written in past tense. But if we decide against, I would like to document why. Same if we decide in favour of requiring Node v7.6+_ | ||
|
||
Node 7.6 introduced native support for async/await. For asynchronous code like much of Hubot is, this is a very nice feature. It makes the code more readable and hence more accessible for contributors which is a main objective of the Hubot community. | ||
|
||
The reason we decided against it is that the complexity of Hubot core is not too high, the code can be made very readable even when using Promises. Once the one-time conversion from CoffeeScript to JavaScript is done, only smaller parts of the code base will be touched, the effect of async/await vs. promises will be limited. | ||
|
||
More importantly, we would make it much harder for existing users to upgrade to the new Hubot version. Hubot is a widely used project and having a clear upgrade plan with a reasonable pace is critical. If we only drop support for versions which are no longer maintained we can align with Node’s [LTS schedule](https://github.com/nodejs/LTS#readme) so we can avoid a recurring discussion of when to drop support for what versions once and for all. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you link to this?