JavaScript

[gr2m]

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

• [x] Update CONTRIBUTING.md, remove mention of CoffeeScript (https://github.com/github/hubot/pull/1349)
• [x] Check in with https://github.com/github/hubot/issues/1338
• [x] add an automated end-to-end test for hubot scripts written in CoffeeScript (https://github.com/hubotio/hubot/pull/1360)
• [x] add tests for require('hubot/es2015.js') (https://github.com/hubotio/hubot/pull/1359)
• [ ] rename src/ to lib/ when everything else is done

[gr2m]

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]

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]

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.

[gr2m]

Unfortunately I run into a new problem with the CoffeeScript-compatible export of "classes" mentioned above

While CoffeeScript can use AdapterCompatibleWithCoffeeScript for own extensions, JavaScript cannot with the class keyword. So this become tricky, in a nutshell we can have either CoffeeScript’s class or JavaScript’s class compatibility – not both :(

We could move the logic to export the classes in a CoffeeScript-compatible way into /index.js so that the built-in adapters shell and campfire could still extend the adapter class while external modules would get the CoffeeScript-compatible versions when doing require('hubot'). But that also means that if someone wants to build their scripts in es2015, then it won’t work unless they directly require the respective class, e.g. require('hubot/src/adapter'), but I wouldn't officially support that, paths like hubot/src/adapter are internal implementation details and we should be able to change them if needed without breaking the eco system. (that being said, hubot-mock-adapter does exactly that. It would be helpful to find out how many others do the same).

I’d still like to move forward with the CoffeeScript -> JavaScript conversion without breaking existing scripts. What we could do is to add a new file that people can require which gives them es2015-compatible class exports. We can deprecate that in future and remove it eventually. I don’t think it would be too much of an overhead. It would be the exact same logic as /index.js without the CoffeeScript-compatibility workaround from my comment above. The only thing we would to do is to update existing scripts that require deep paths like hubot-mock-adapter does and change require('hubot/src/adapter') to require('hubot').Adapter. A code search currently returns 107 results for "hubot/src/adapter", people also directly require other classes.

Any thoughts?

See https://github.com/github/hubot/pull/1347/commits/a1e6b1d3e822fa019c4f9cbbef4514d268ef948b for the implementation of the suggestion above

[gr2m]

@hubotio/maintainers I think I addressed all feedback and would suggest we merge in the proposal and move on with the pull request at https://github.com/github/hubot/pull/1347. I would move all TODOs from the proposal to the pull request description to keep everyone up-to-date with the progress.

This is blocking a lot of other efforts to make the Hubot project more contributor / maintainer friendly and is also blocking existing and new pull requests, so I’d like to move forward as fast as possible and if possible move things into follow up issues / PRs

[bkeepers]

Unfortunately I run into a new problem with the CoffeeScript-compatible export of "classes" mentioned above

Dang, CoffeeScript not support ES2015 classes is a huge bummer. The workarounds are clever, so 👍 if that works. The explanation of the issue and the workaround should probably be documented in the proposal.

I think I addressed all feedback and would suggest we merge in the proposal and move on with the pull request at github/hubot#1347.

The process docs currently say: "Once a proposal has been implemented, the Pull Request will be merged." I know that feels a little weird to leave a "proposal" open until the implementation is done, but I think that keeps all of the discussions and documentation in one place.

I think it make sense to leave this open until we finish the implementation, including a release and docs, since some of the details may change.

[gr2m]

The explanation of the issue and the workaround should probably be documented in the proposal.

done: https://github.com/hubotio/evolution/blob/4/javascript/_drafts/javascript.md#proposed-solution

I think it make sense to leave this open until we finish the implementation, including a release and docs, since some of the details may change.

My fault, I misunderstood the process, all clear now

[gr2m]

My PR is done and ready for review: https://github.com/github/hubot/pull/1347

What would be great is if people could help me test my PR against existing adapters and other 3rd party libraries which depend on hubot to make sure they all still work. I’ve tested Slack and it alls seems to work fine. I’ve documented the process of testing at https://github.com/github/hubot/pull/1347#test. Thanks 🙏

[gr2m]

pull request was merged: https://github.com/hubotio/evolution/pull/4#issuecomment-305557470. Will now look into the open todos / follow ups

[gasolin]

Nice to see ES6 porting is moving on! @gr2m you can also take a look on https://github.com/gasolin/webbybot with is a fork from hubot and focus on translate it to es6 syntax. Test cases are transferred first to make sure everything is still works fine in coffescript / es6 port.

Here's the related blog post https://blog.gasolin.idv.tw/2016/03/14/How-we-ported-Hubot-from-Coffeescript-to-ES6/

[gr2m]

Where can the README/docs for the current Hubot v2 remain accessible

I would suggest we create a 2.x branch and link https://github.com/github/hubot/blob/v2.x/docs/index.md directly from our documentation so people could still look up the old docs.

[gr2m]

@technicalpickles brought up problems with peerDependencies in existing hubot scripts that we have to address before releasing hubot@3: https://github.com/hubotio/hubot/issues/1057

[gr2m]

I’ve added a release plan for [email protected] to address concerns from hubotio/hubot#1057. See the latest version in the proposal at https://github.com/hubotio/evolution/blob/4/javascript/_drafts/javascript.md#release-hubot300

[gr2m]

Also added another todo:

Add documentation for developers of adapters and scripts. If they want to use modern es2015 classes, they must require hubot >= 3 in peerDependencies and require('hubot/es2015'). If they want to remain compatible with hubot@2 then they can’t use es2015 classes. We should document that somewhere and explain it very clearly.

[technicalpickles]

pull request was merged: #4 (comment). Will now look into the open todos / follow ups

It feels weird to me that the hubot PR was merged before we'd 'accepted' this proposal, when I had requested changes. I was offline the week that a lot of work was done, and this was merged, but I didn't see discussion around 'dismissing' my feedback since it had been addressed.

The process docs currently say: "Once a proposal has been implemented, the Pull Request will be merged." I know that feels a little weird to leave a "proposal" open until the implementation is done, but I think that keeps all of the discussions and documentation in one place.

I think it make sense to leave this open until we finish the implementation, including a release and docs, since some of the details may change.

I think this is highlighting a problem with how the evolution flow is intended to work. In particular, if I've requested changes to the proposal (via PR), then it's never blocked from

Some ideas:

• move discussion to an issue (if there's not already) once the plan has been approved
• update the proposal after it's been merged if implementation details
• make it clear that the proposal is what is determined at time of planning, and details will change
• consider enabling issues on this repo so they can be tracked here, but be pretty strict about what should be left open

[timothywellsjr]

Hey guys while running script/server I am getting the error below. I have the hubbermoji.yml file not sure why it cannot find the file?

Have you seen this error message before? It seems to be looking for a JS file and not the .yml data file. https://github.com/hubotio/hubot/blob/master/src/robot.js#L376 @mistydemeo suggested that the new code path possibly could be having issues loading files that aren’t actually JS modules.

No history available
Hubot> [Tue Jun 20 2017 17:22:00 GMT-0400 (EDT)] ERROR Unable to load /Users/timothywellsjr/github/hubot/bin/hubot-classic/scripts/hubbermoji: Error: Cannot find module '/Users/timothywellsjr/github/hubot/bin/hubot-classic/scripts/hubbermoji'
    at Function.Module._resolveFilename (module.js:469:15)
    at Function.Module._load (module.js:417:25)
    at Module.require (module.js:497:17)
    at require (internal/module.js:20:19)
    at Robot.loadFile (/Users/timothywellsjr/github/hubot/bin/hubot-classic/node_modules/hubot/src/robot.js:353:22)
    at fs.readdirSync.sort.map.file (/Users/timothywellsjr/github/hubot/bin/hubot-classic/node_modules/hubot/src/robot.js:376:52)
    at Array.map (native)
    at Robot.load (/Users/timothywellsjr/github/hubot/bin/hubot-classic/node_modules/hubot/src/robot.js:376:35)
    at Shell.loadScripts (/Users/timothywellsjr/github/hubot/bin/hubot-classic/node_modules/hubot/bin/hubot.js:115:9)
    at Shell.g (events.js:291:16)
    at emitNone (events.js:86:13)
    at Shell.emit (events.js:185:7)
    at loadHistory (/Users/timothywellsjr/github/hubot/bin/hubot-classic/node_modules/hubot/src/adapters/shell.js:47:19)
    at loadHistory (/Users/timothywellsjr/github/hubot/bin/hubot-classic/node_modules/hubot/src/adapters/shell.js:119:12)
    at Shell.run (/Users/timothywellsjr/github/hubot/bin/hubot-classic/node_modules/hubot/src/adapters/shell.js:40:5)
    at Robot.run (/Users/timothywellsjr/github/hubot/bin/hubot-classic/node_modules/hubot/src/robot.js:639:18)
    at Object.<anonymous> (/Users/timothywellsjr/github/hubot/bin/hubot-classic/node_modules/hubot/bin/hubot.js:112:7)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (/Users/timothywellsjr/github/hubot/bin/hubot-classic/node_modules/coffee-script/lib/coffee-script/register.js:45:36)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)
    at Module.require (module.js:497:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> (/Users/timothywellsjr/github/hubot/bin/hubot-classic/node_modules/hubot/bin/hubot:2:3)
    at Object.<anonymous> (/Users/timothywellsjr/github/hubot/bin/hubot-classic/node_modules/hubot/bin/hubot:4:4)
    at Module._compile (module.js:570:32)
    at Object.exports.run (/Users/timothywellsjr/github/hubot/bin/hubot-classic/node_modules/coffee-script/lib/coffee-script/coffee-script.js:173:23)
    at compileScript (/Users/timothywellsjr/github/hubot/bin/hubot-classic/node_modules/coffee-script/lib/coffee-script/command.js:224:29)
    at compilePath (/Users/timothywellsjr/github/hubot/bin/hubot-classic/node_modules/coffee-script/lib/coffee-script/command.js:174:14)
    at Object.exports.run (/Users/timothywellsjr/github/hubot/bin/hubot-classic/node_modules/coffee-script/lib/coffee-script/command.js:98:20)
    at Object.<anonymous> (/Users/timothywellsjr/github/hubot/bin/hubot-classic/node_modules/coffee-script/bin/coffee:7:41)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)
    at Module.runMain (module.js:604:10)
    at run (bootstrap_node.js:394:7)
    at startup (bootstrap_node.js:149:9)
    at bootstrap_node.js:509:3

C02RK221FVH8:hubot-classic timothywellsjr$ 

[mistydemeo]

Yeah, in our current bot this data file lives alongside coffeescript and javascript scripts. Does the new loader try to load anything that's in there, not just js/coffee files?

[technicalpickles]

@timothywellsjr @mistydemeo I filed https://github.com/hubotio/hubot/issues/1355 to work through that. It does seem look like a regression though.

[gr2m]

update

• [x] add an automated end-to-end test for hubot scripts written in CoffeeScript (https://github.com/hubotio/hubot/pull/1360)
• [x] add tests for require('hubot/es2015.js') (https://github.com/hubotio/hubot/pull/1359)

[gr2m]

Question: shall we remove loading scripts from hubot-scripts.json as it deprecated in 2.x and announced to be removed in 3.x? https://github.com/hubotio/hubot/blob/d53e399fa0a6a2f0b441aafb869d1640d12785e0/bin/hubot.js#L130-L192

[gr2m]

Not a specific line item, and I may be missing it somewhere else, but before this is done, we'd also need some way to easily access old, existing documentation that is written for coffeescript.

@technicalpickles it’s one of the open todos in the PR description above

Where can the README/docs for the current Hubot v2 remain accessible? See https://github.com/hubotio/evolution/pull/4#discussion_r119500396

The simplest would be to create a 2.x branch and to link to https://github.com/hubotio/hubot/tree/master/docs. I don’t know how we currently deploy to ttps://hubot.github.com/docs/, if we could somehow deploy the docs of a 2.x branch to https://hubot.github.com/v2/docs/ that’d be great

[technicalpickles]

shall we remove loading scripts from hubot-scripts.json as it deprecated in 2.x and announced to be removed in 3.x?

I'd be 👍 to that. We'd have to make sure to update the generator too https://github.com/hubotio/generator-hubot/pull/91

I don’t know how we currently deploy to https://hubot.github.com/docs/

It's powered by GitHub pages, coming from a private repo. This repository is included as a submodule under docs, so it generates the right thing. It'd be possible to apply that same pattern to another path.

[technicalpickles]

@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]

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?

[technicalpickles]

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

[gr2m]

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]

I think I missed that we were using the hubot-mock-adapter before now. I see https://github.com/blalor/hubot-mock-adapter/pull/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]

@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]

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]

Ping.