Javascript regression: Hubot tries to load all files in script directories, even if they aren't Javascript

[technicalpickles]

@timothywellsjr posted on the Javascript evolution with a report that hubot was trying to require a YAML in scripts. @mistydemeo asked if the updated hubot is trying to load all files, not just javascript.

It seems like, yes, it is. Here's a dive through the relevant codes:

• bin/hubot tries to load scripts: https://github.com/hubotio/hubot/blob/master/bin/hubot.js#L115
• robot.load finds all files in the scripts directory, then calls robot.loadFile: https://github.com/hubotio/hubot/blob/master/src/robot.js#L376
• robot.loadFile tries to require

Compare this to the previous behavior of loadFile which checks if it's a supported require type: https://github.com/hubotio/hubot/pull/1347/files#diff-740956d8fdad73632c4937d631b7c79bL353

cc @gr2m

[technicalpickles]

This would need to be tested, but if anyone is running into it, you could try moving the files out of the scripts directory as a work around. That might require other code changes to account for different paths.

[gr2m]

Thanks for the research into the problem @technicalpickles, it sounds right. I think we can check for just js and coffee extensions as we don’t support others right now? We can add that check back into loadFile method at

hubot/src/robot.js

Line 352 in d3df713

try {

[technicalpickles]

@gr2m I'd use the same check that was there, ie if require.extensions[ext] to preserve existing behavior.

I checked the require.extenisons docs and it says it's deprecated, but not going anywhere. I don't think we need to tackle that as part of this 3.0 release for javascript support.

[gr2m]

sounds good

[gr2m]

@technicalpickles I’ll prepare a PR today

[gr2m]

sorry didn’t manage to yesterday and will be in a meeting all day today. I’ll send a PR tomorrow

[gr2m]

@technicalpickles @timothywellsjr @mistydemeo I started a PR that should fix the problem:
#1357 – could you give it a try please?

[timothywellsjr]

👍

[timkinnane]

Had a question on how to resolve the related issue of adding a warning for when unsupported files are rejected from loading. I was going to address it by adding something like:

this.logger.warning(`${filename} uses unsupported extension, only ${Object.keys(require.extensions).join(', ')} are accepted`)

But getting the list of accepted extensions, I found that it accepts .js, .json and .node. If coffee-script is also required, it will also allow .coffee, .litcoffee and .coffee.md.

Now, most of those should not be accepted by the .loadFile method AND the require.extensions is also depreciated, requiring a // eslint-disable-line at the end of the line just to pass standardjs linting.

Due to the above, why not just use a fixed array of accepted types, being just ['.js', '.coffee']?