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

JavaScript #4

Open
wants to merge 38 commits into
base: master
Choose a base branch
from
Open

JavaScript #4

wants to merge 38 commits into from

Conversation

gr2m
Copy link

@gr2m gr2m commented May 31, 2017

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.

Read the proposal 👀

Open todos / follow ups

- [ ] Convert test files from CoffeeScript to JavaScript.
- [ ] update package.json
- [ ] add script for JavaScript linting
- [ ] Update documentation
Copy link
Contributor

Choose a reason for hiding this comment

The 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

@gr2m gr2m added the In Review label May 31, 2017
### 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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The author of decaffeinate uses hubot code base for his tests of achievement, it's worth citing https://github.com/decaffeinate/decaffeinate#status and the hubotio/hubot#1138 issue.

Copy link
Author

@gr2m gr2m May 31, 2017

Choose a reason for hiding this comment

The 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


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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debian users that need modern node version have 2 main easy options:

Copy link
Author

Choose a reason for hiding this comment

The 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

Copy link
Member

@technicalpickles technicalpickles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great writeup, thanks for putting it together!

I have tons of questions and feedback 😁 It is mostly from experience with working on hubot, and also to make this specific enough that it can be implemented by someone that isn't @gr2m


## Motivation

This evolution proposal is part of point 3 of the Hubot 3.0 milestone:
Copy link
Member

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?


## Detailed process

The steps of the conversation will be
Copy link
Member

Choose a reason for hiding this comment

The 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?


Make sure the existing tests (still written in CoffeeScript) run against the new JavaScript.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/trough/through/


Make sure the existing tests (still written in CoffeeScript) run against the new JavaScript.

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.
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct


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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😍

This sounds like a great way to build confidence in the conversion.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a with with duplication in that paragraph.


### 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`.
Copy link
Member

Choose a reason for hiding this comment

The 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.


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`.

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.
Copy link
Member

Choose a reason for hiding this comment

The 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).


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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a relevant link for this, ie tweet, documentation, etc?


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.

Besides, the current Hubot does not use a single promise in its core implementation at this point.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc hubotio/hubot#1338 which is talking about adding it


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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this sentiment.

I'm not familiar enough with async/await yet, but would it possible to still take advantage of these support in the core? That is, can hubot users use it even if the core wasn't specifically written for it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, Hubot user can take full advantage for the Node version they run their own Hubot instance in. If Hubot core adds Promise-based APIs then users which run their Hubot in Node 7.6+ can use async/await as these are sugar APIs to work with Promises. At the same time the core would continue to work with Node v4, so we don’t break Hubot instances that require this older Node version. Does that answer your question?

gr2m added 2 commits June 1, 2017 15:45
remove obsolete code in example
gr2m referenced this pull request in hubotio/hubot Jun 1, 2017
Problem is that es6 classes break `hubot-mock-adapter` because it does a custom inheritance workaround which does not call the Adapter class with the `new` keyword
@gr2m
Copy link
Author

gr2m commented Jun 1, 2017

I’m running into a bit of a blocker: when using the class keyword in the JavaScript version of Hubot, the way existing adapters inhert from Adapter would break because they are doing this

MyAdapter.__super__.constructor.apply(this, arguments);

which fails with TypeError: Class constructor Adapter cannot be invoked without 'new'. There is a related issue here. Slack also extends the Message class, probably others do so, too.

I’m still thinking about how to find the best balance between readable, easy to contribute/maintain code on the one and as few breaking changes on the other side.

Let me know if you have any thoughts on this :)

@technicalpickles
Copy link
Member

technicalpickles commented Jun 1, 2017

which fails with TypeError: Class constructor Adapter cannot be invoked without 'new'. There is a related issue here. Slack also extends the Message class, probably others do so, too.

I did a search for this error (ie javascript class constructor cannot be invoked without new), and seeing it come up quite a bit. There's a lot of github issues around specific libraries/tools. I think http://raganwald.com/2014/07/09/javascript-constructor-problem.html looks like a decent description of the problem with some proposed solutions.

I’m still thinking about how to find the best balance between readable, easy to contribute/maintain code on the one and as few breaking changes on the other side.

Initially, I'd be most concerned with interoperability with existing code, unless we are committing to updating all the existing adapters as part of this work. I suspect less readable, harder to contribute to code can be confined to specific parts of the code base. Combine generous amounts of code comments about what it is trying to achieve, I think that is a good compromise.

That said, another approach is to figure out the most readable/maintainable thing, and then go back to add those bits to ensure better interop.

And one last thought: it should be possible to write tests cases that work on master, but fail in this case. That could help us know when the problem is fixed.

@gr2m
Copy link
Author

gr2m commented Jun 1, 2017

I’ve to log off for today, just want to share what I’m looking into right now.

I haven’t fully tested it yet, but I think if we add sth like this to the footer before we return the classes that others inherit from, we should be fine:

const inherits = require('util').inherits
const EventEmitter = require('events').EventEmitter
class Adapter extends EventEmitter {
  // implementation of Adapter
}

function AdapterCompatibleWithCoffeeScript () {
  return new (Function.prototype.bind.apply(Adapter, [ null ].concat([].slice.call(arguments))))
}
inherits(AdapterCompatibleWithCoffeeScript, Adapter)

module.exports = AdapterCompatibleWithCoffeeScript

It’s not very pretty, but it might be a good compromise for the time being, so we can move forward with JavaScript implementation while remaining compatibility with existing adapters.

@technicalpickles
Copy link
Member

@gr2m I just tried out npm install --save hubot@next on a bot I built last week, where I was using hubot-test-helper:

npm WARN unmet dependency /Users/technicalpickles/src/parrotbot/node_modules/hubot-test-helper requires hubot@'>= 2.6.0 < 3' but will load

I recall that there were some issues with hubot-test-helper related to inheritence and how it uses classes. I think this might be common enough of a module to make sure it works with hubot 3 & javascript.

@gr2m
Copy link
Author

gr2m commented Jun 29, 2017

I’m removing hubot-test-helper right now from the yeoman script generator. We don’t use it in our scripts any more, the tests with it have been failing by default.

Do you have an app which is using it right now with passing tests?

gr2m added 4 commits June 28, 2017 19:38
complete list of removed default scripts installed by generator
all work is done, now on to testing: `npm install --global generator-hubot@next`
@technicalpickles
Copy link
Member

I don't, but could get it running in hubot-for-hubot pretty easily.

@gr2m
Copy link
Author

gr2m commented Jun 30, 2017

from my experience, it’s better to keep tests less "magical" and more verbose. The hubot-test-helper is helpful when you setup test and you know exactly what you are doing, but for people new to the project they would need to check it out as well. The way we did it e.g. at https://github.com/hubotio/hubot-rules/blob/master/test/rules-test.js has more code, but also is clear simpler to follow for a new contributor, I think

@technicalpickles
Copy link
Member

I think I missed that we were using the hubot-mock-adapter before now. I see hubotio/hubot-mock-adapter#7 that is being used there, and that https://www.npmjs.com/package/hubot-mock-adapter-v3 was released using that. Would you mind moving the fork of hubot-mock-adapter to the hubotio org?

There is documentation that recommends using hubot-test-helper, so we'll want to update that too: https://github.com/hubotio/hubot/blob/master/docs/scripting.md#testing-hubot-scripts

@technicalpickles
Copy link
Member

@gr2m was asking for folks to test the new release. I just ran through it without any problem:

npm install -g generator-hubot@next
mkdir hubot-next
yo hubot
bin/hubot

I tested a few scripts I knew existed (ping, help, the rules), and they responded.

Only thing of note I saw was a message the first time launching bin/hubot that a package-lock.json was created and should be committed. That is going to happen for newer npm releases, but that is a general improvement for the generator, and not really specific to the Javascript evolution.

@gr2m
Copy link
Author

gr2m commented Jul 6, 2017

Only thing of note I saw was a message the first time launching bin/hubot that a package-lock.json was created and should be committed. That is going to happen for newer npm releases, but that is a general improvement for the generator, and not really specific to the Javascript evolution.

This will be fixed once we remove the bin/hubot from the generator and replace it with a npm start which just runs the hubot binary provide by the hubot package

@joeyguerra
Copy link
Member

Ping.

@gr2m
Copy link
Author

gr2m commented Jun 18, 2019

I’m no longer working on Hubot I’m afraid. I cannot help I’m afraid

@joeyguerra
Copy link
Member

joeyguerra commented Jun 18, 2019

Can I maintain it?

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

Successfully merging this pull request may close these issues.

9 participants