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

Fail fast for unsupported Node versions #174

Closed
3 tasks done
aoberoi opened this issue Apr 30, 2019 · 2 comments · Fixed by #189
Closed
3 tasks done

Fail fast for unsupported Node versions #174

aoberoi opened this issue Apr 30, 2019 · 2 comments · Fixed by #189
Labels
enhancement M-T: A feature request for new functionality good first issue Good for newcomers

Comments

@aoberoi
Copy link
Contributor

aoberoi commented Apr 30, 2019

Description

Bolt has a fairly high Node version requirement (v10 LTS). We think this is okay for this project's first release, and will align to the Node Slack SDK Support Schedule in the future.

Since the version requirement is relatively high, and Bolt is meant to be approachable for relatively new programmers, any guidance regarding not meeting requirements would be beneficial. While version detection is normally considered a bad practice, and feature detection is usually preferred, we feel that in this case its a reasonable solution.

The requirement is to throw an Error when initializing App and the process.versions.node is less than major version 10. The Error should have a very clear and helpful message about not meeting the requirement and how the developer can update their current version.

Requirements (place an x in each of the [ ])

  • I've read and understood the Contributing guidelines and have done my best effort to follow them.
  • I've read and agree to the Code of Conduct.
  • I've searched for any related issues and avoided creating a duplicate issue.
@aoberoi aoberoi added enhancement M-T: A feature request for new functionality good first issue Good for newcomers labels Apr 30, 2019
@colestrode
Copy link

I ran into this today, it looks like when the app initializes (i.e., when the constructor is called) would be too late. Here is the error I saw when trying to start my app using Node v8.11.0:

/Users/cfurfarostrode/src/projects/tldr-bot/node_modules/@slack/bolt/dist/middleware/builtin.js:207
const slackLink = /<(?<type>[@#!])?(?<link>[^>|]+)(?:\|(?<label>[^>]+))?>/;

SyntaxError: Invalid regular expression: /<(?<type>[@#!])?(?<link>[^>|]+)(?:\|(?<label>[^>]+))?>/: Invalid group
    at Object.<anonymous> (/Users/cfurfarostrode/src/projects/tldr-bot/node_modules/@slack/bolt/dist/middleware/builtin.js:207:19)
    at Module._compile (module.js:652:30)
    at Object.Module._extensions..js (module.js:663:10)
    at Module.load (module.js:565:32)
    at tryModuleLoad (module.js:505:12)
    at Function.Module._load (module.js:497:3)
    at Module.require (module.js:596:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/Users/cfurfarostrode/src/projects/tldr-bot/node_modules/@slack/bolt/dist/App.js:12:19)
    at Module._compile (module.js:652:30)

That error happens when importing from the ./middleware/builtin module. During import/require the module gets evaluated and older Node versions have a hard time with the regex on this line: https://github.com/slackapi/bolt/blob/master/src/middleware/builtin.ts#L248

If you want to fail fast, you'll need to test the node version in App.ts before the import statements. It's probably worth adding something to the README too about supported Node versions.

FWIW I think supporting Node 10+ is fine, Node 8 leaves TLS at the end of this year, so it doesn't seem worth adding backwards compatibility especially once this issue is fixed.

@clavin
Copy link
Contributor

clavin commented May 2, 2019

I think if the entire package is scoped to Node 10+ (and in light of the above finding), it might make the most sense to put this warning at the top of index.ts, similar to how create-react-app does for the same constraint on Node 8+.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement M-T: A feature request for new functionality good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants