Integrate Travis for PRs and ensure passing

[brettz9]

As discussed in #37 , Travis CI isn't currently passing due at least to the fact that ESLint 7 expects Node 10+. It can get increasingly difficult to ensure testing tools are downgraded through conditionals in the CI logic. Are you ok with a breaking change to require engines of Node 10+ (and removing the earlier versions from Travis)?

If so, do you also want me to switch from buble to babel so we can ensure the built files only need to use Node 10+ features (by using @babel/preset-env)?

If such a PR is ok, it'd still be nice to have this issue for tracking your adding of any Travis integration into Github so any future additions could be clearly visible as to how failing (and allowing one to investigate how to remedy). As mentioned, I think you'd just need to go to https://github.com/marketplace/travis-ci and follow the instructions there.

[d3x0r]

https://github.com/d3x0r/JSON6/commit/33aed761ab3fa8633d9f2c37ba146602c1b68713

... waiting for build....

[d3x0r]

https://travis-ci.org/github/d3x0r/JSON6/jobs/692032999#L674

[brettz9]

Yeah, that's due to the ESLint version issue I mentioned. Do you want to drop support for older versions? Conditional logic can be added to downgrade the ESLint version, but it can get complicated.

[brettz9]

Also, while linking to Travis is cool, and having it work over there is good, it'd be nice to have the full Github integration per following the steps at https://github.com/marketplace/travis-ci (despite it mentioning a "trial", it does not cost anything for open source projects as this one per the chart on the bottom left of the page). The Github integration will allow one to see the Travis status directly within PRs...

[brettz9]

Nevermind, I see it is integrated now, cool, thanks!

[brettz9]

Just let me know if you are ok dropping Node < 10 support. Many projects have done so with such versions having reached Node's end-of-life support. But if you really need to have it, I can see about adding conditional logic to the Travis file, so it will try to use an older version of ESLint, nyc, mocha, chai, etc. when running tests--but there is a chance even this won't work if the test files are using features that the older build tools can't compile.

[d3x0r]

< 10 ? is that really when => was available? I thought that was 6?
node 7.0.0-32 works...

[brettz9]

You're right that arrow function were available earlier, but Travis is just showing the first error there for that 0.10 build. If you look at the 8.0 build, e.g., at https://travis-ci.com/github/brettz9/JSON6/builds/168512783 , you can see it is failing because of a different feature used by ESLint 7, the optional catch binding: https://travis-ci.com/github/brettz9/JSON6/jobs/340801806#L222

(There is another issue though that for some reason, there is an error showing on your Travis builds (including Node 10+) which aren't showing for some reason on mine which I'll have to investigate. But in any case, it is certain that ESLint 7 requires Node 10+, also per their release notes.)

[d3x0r]

Ya okay I see; the 10 hadn't actually completed yet so I didn't see any successes earlier :) I have no problem with that - the intern V8 engine is incompatible before 10 too...

[brettz9]

Thanks for the checks/commit.

I think the only remaining questions here then are:

1. Shall I switch engines to Node 10+ since we are no longer checking that the code still works pre Node 10?
2. If so, do you also want me to switch from buble to babel so we can ensure the built files only need to bother polyfilling features missing in Node 10+ (by using @babel/preset-env)?

[d3x0r]

Uhmm. I included the 10 truncation in the pull requestion with travis.yml modifications.

That's probably a good idea to only have to fill what's needed.

[d3x0r]

So, I was going to update the readme to include notes about required versions of things/required behaviors... and it would be something like '

Version Support

The lib/json6.js' itself works back to node6, and just requires let, const and arrow functions. The tools to compactify and verify it, however, require Node 10+.

[d3x0r]

I know; 7 is a non support - it's just where I ended up starting... so that's really 8 ?

[d3x0r]

It doesn't even use arrow functions. (lib/json6.js)

[brettz9]

Regarding the build failing badge... Caching on Github can sometimes be slow to update, but in any case, I see now that it is passing.

Re: README, that's fine by me, but my PR just now to add engines of Node 10 would prevent people using a pre-Node 10 version if using the engine-strict npm config. I can remove that part of the PR.

But since one really only knows for sure that the latest code will pass on the machines tested in Travis, I'm not sure the removal of setting engines to Node 10+ should be made--as much as I also tend to favor supporting as low of a version as possible.

Besides the assurances it gives that we only claim support for what CI is testing, it may in fact be nice to take advantage of more recent features in source going forward, allowing one to use things like String.prototype.includes which older Node doesn't support and which Babel doesn't polyfill without need for requiring users to use core-js (to add those polyfills for includes and such).

Your call though of course.

[brettz9]

Re: Node 7/8, not sure what you mean. If you mean whether one must have Node 8 instead of Node 6, I think Node 6 would work with the code as is--it does support arrow functions, for example, as per https://node.green/ . But only really know for sure if checking (manually if not in Travis--but may be work involved to convert code if tests use newer features).

[brettz9]

And let/const, are supported going back to early versions, though not in all details except with Node 6+; see https://node.green/#ES2015-bindings

[d3x0r]

Why do we have to include these?

    core-js: "latest",
    chai: "^3",
    acorn: "^6",

I understand these
rollup: "^1.20", eslint: "latest", nyc: "latest", mocha: "^3",

And I guess these... "@rollup/plugin-buble": "latest", "@rollup/plugin-commonjs": "latest", "@rollup/plugin-node-resolve": "latest", "@rollup/plugin-strip": "latest",

[d3x0r]

String.prototype.includes but, if I'm not already using it, what would it be used for? Using a feature 'just cause' doesn't make a lot of sense to me.

[brettz9]

Yeah, I'm not speaking about adding it for no reason--just mean it can be nice if one finds the use. (Seems there's not even indexOf currently.)

[d3x0r]

ya there's no searching... it doesn't have to lookup anything....

[brettz9]

chai is what allows you to use expect within the Mocha tests (otherwise you have to use Node's assert if you don't want an additional dep.).

I seem to recall acorn was a peer dep. of some other package, but I can check. Same too maybe with core-js. Can investigate.

[d3x0r]

https://github.com/d3x0r/JSON6#requirements if you find on the others ...

ya they are peer deps... and I didn't recall off hand either.

[brettz9]

Yeah, acorn is a peer dependency of acorn-jsx, giving warnings at the very least if not included. acorn-jsx is required because it is a dependency of espree and espree is a dep. of eslint. (espree itself requires a supported version of acorn as a direct dependency, so it will be installed regardless, but I guess there will otherwise be a warning when doing a JSON6 install because acorn-jsx has it as a peer dep. insisting apparently that we directly add it ourselves as well).

We are using core-js directly in /build/es5.js (Unicode string polyfills).

[d3x0r]

Nice. Lots of span on pushses - should the 'console.log( 'GOt:' )' be stripped from tests? (yes?)

[brettz9]

SGTM...

[brettz9]

Btw, I don't see build/unicode.js being used for anything.

[d3x0r]

It's not; I deprecated any checks for 'unicode' identifiers... that really only applies on stringification; and this became a lot more liberal about unquoted things it accepts as field identifiers.
cleaned tests.

[brettz9]

I think this is ok now, so closing.