tmijs / tmi.js

💬 Javascript library for the Twitch Messaging Interface. (Twitch.tv)
https://tmijs.com
MIT License
1.54k stars 217 forks source link

About that node v7 support #406

Closed FoseFx closed 3 years ago

FoseFx commented 4 years ago

Actual behaviour: The docs clearly state:

This module currently supports Node.js 7.x and every browser that supports WebSockets

In the package.json we see:

"engines": {
    "node": ">=7.10.1"
}

c28bc80880b432e435fe3d2d09309e35563aa3e3 broke this by adding versions of mocha and nyc which depend on "find-up", a package that uses es6 syntax. Sure this only relevant if you want to test tmi..js on your own using node v7 and (maybe) does not break compat for the package itself. But here is the problem: When CI can't test in on v7, how is v7 compat guaranteed? And should it?

Expected behaviour: Either fix the issues related to tests failing on v7 or ditch v7 (I mean LTS of node is 12.x right now).

Error log:

nyc mocha -- --require should --exit

/home/travis/build/tmijs/tmi.js/node_modules/find-up/index.js:29
        const foundPath = await runMatcher({...options, cwd: directory});
                                            ^^^
SyntaxError: Unexpected token ...
    at createScript (vm.js:53:10)
    at Object.runInThisContext (vm.js:95:10)
    at Module._compile (module.js:543:28)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)
    at Module.require (module.js:498:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> (/home/travis/build/tmijs/tmi.js/node_modules/nyc/lib/config-util.js:4:16)
AlcaDesign commented 4 years ago

Thanks for testing that! Spreading object properties isn't available until Node 8.3.0, according to node.green and mdn. tmi.js@1.x is sort of in legacy mode but the dev dependencies had a vulnerability. I needed to update from istanbul to nyc as they recommend. I've checked the minimum Node version myself whenever newer syntax is added to tmi.js code but I understand that's not enough.

Testing tmi.js against Node 7.10.1, 8.3.0, 9.11.3 all failed with either the spread error or an error about Unexpected token import. The latest version of 10.x, which is 10.21.0 did actually run the test and passed. I'm comfortable upping the minimum Node support to >=10.21.0 as it's in maintenance and not yet end-of-life. Then by April 2021 it will need to be upped again to Node 12.x for tmi.js 1.x if it's still around.

FoseFx commented 4 years ago

tmi.js@1.x is sort of in legacy mode

Is there another version in development? The v2 branch seems to be stale

AlcaDesign commented 3 years ago

Fixed in #431