compile using babel for compatibility with older versions

[brettz9]

This should work (to close #95), but still need to add matrix logic to travis to install older nyc, etc. to confirm build is working

[brettz9]

The latest Rollup plugins don't work on older Node. I've avoided the build step for those versions now, but now tests are failing as Babel is insufficient as Babel doesn't polyfill globals like Object.entries by default. Looking to add plugin to transform now.

[brettz9]

While I've gotten Object.entries transformed, it seems the Babel plugin for dot-all (the s flag) will only handle transforming when within regex literals, so one test is failing on Node 6. I could perhaps add core-js if it has a RegExp polyfill that can handle the flag.

As far as earlier Node versions, @babel/register itself does not work on older Node. I can look to using babel-register, but then the dev. code will be using a less up-to-date tool. (I could perhaps see about running a different test script on Travis.)

[brettz9]

So babel-register doesn't work as it doesn't seem to be able to understand the import statements as @babel/register does.

I could try retooling the tests to point to the dist copy if that is desired.

Alternatively, we could just avoid adding pre-Node 6 versions to travis and get out a patch earlier, trusting that babel is working (after this PR) as advertised.

As far as the single Node 6 test failure, from what I can tell, the core-js polyfill for the RegExp constructor doesn't properly handle the dotall flag as I was still getting errors despite including the polyfill and successfully confirming it was being used. But older esquery dependencies shouldn't have been relying on the 's' flag anyways, as it had not been supported in esquery previously (nor would older environments support it), so while it would be good to see what we can do to get it to work under core-js, I think we could also drop Node 6 from Travis for now too (or ignore its result), given that this one failure is expected without a proper polyfill.

Suggested steps forward?

[brettz9]

Btw:

1. For engines, I set a minimum of 0.10 based on this comment, though let me know if you want to test against 0.1.14, as this is, IIRC, the earliest usable).
2. FWIW, for rollup-plugin-babel, I didn't exclude node_modules from babelification in case the latest estraverse is using anything incompatible.
3. I've filed [RegExp] dotAll flag zloirock/core-js#792 to see about corejs getting full dotall support.

[mbroadst]

@brettz9 what if we use a modern version of node to build everything, and then only run the tests in legacy versions of node? I was able to run the esquery tests just fine in Node 4 with the work present in your initial PR, so we might be solving for the wrong thing at this point.

There are a few ways I could imagine doing this:

• I think travis already uses nvm, so you might be able to use that command before running the actual tests
• We could get relatively complex and use build stages and an S3 bucket, where the built assets are fetched from the S3 bucket before each build.

[brettz9]

@mbroadst : Since @michaelficarra had indicated previously that he did not wish to bundle the distributions, yes, I think another way would need to be adopted to build (or obtain) the builds and test against them.

Even if the building issues are addressed, those earlier Node environments will still fail on the dotAll test, though as mentioned, and as a new feature anyways, I think that's one that could be ignored for now.

(Btw, I don't see though how the original PR could have worked for you on Node 4 with the dotAll test. I also don't see how it could not have erred due to a lack of Object.entries unless you were using your own polyfill like core-js. The Object.entries change was especially important as it impacted the build and is not merely testing related. In any case, that is now fixed.)

This is toward the end of my day over here (in China), btw, so you might want to look into devising a build solution on your own if you need a fix for the day.

[mbroadst]

@brettz9 I'm looking into this now.

You're correct that I hadn't got to the Object.entries issues yet, I was just confirming I could load the emitted javascript, thanks for that tip!

I think we still have more work to do here outside of the dotAll test for older distributions. All of the test files are currently using ES6 modules (and likely other newer features), so they will have be transpiled as well in order to run in those environments. Involving so much tooling and transpiling is appearing to have diminishing returns in terms of future maintenance of this codebase.

[brettz9]

By moving toward ESM, I would think it would be better preparing for future maintenance (and is already doing so in allowing for ESM distributions). But yes, it is causing pains at the moment due in part to the fact that the toolers have themselves dropped support.

How about I adapt the tests to avoid ESM in favor of pointing to the compiled dist files (and skip the dotAll test). If @michaelficarra wants the tests in ESM, we can add back the ESM tests in a future breaking version (which drops testing for older versions). But this would allow tests to work for now, while still allowing ESM source and builds.

[mbroadst]

@brettz9 I'm not against moving towards ESM at all, I'm just pondering the changes needed in order to support what's needed here compared to simply cutting a major release now. I think the approach you suggest would work, but we still end up with a travis configuration which is manually replacing dependencies for certain versions of node (etc) which is more what I'm getting at in terms of a maintenance burden.

[michaelficarra]

@brettz9 I am okay with limiting the new functionality to only the supported engines going forward. This means that the library just needs to load and provide at least the 1.1.x functionality on older platforms. Will that help simplify this PR? We can gate tests for newer features behind node version checks.

[michaelficarra]

@mbroadst I never mind a ping. Sorry about the delay, the week of a TC39 meeting is always nuts.

@brettz9 I don't think we need to go through excessive effort to get old versions of node running our tests in CI. What we really care about is that the code we actually distribute will work in these environments. If you've manually tested it by converting the imports to requires and running the tests, that's sufficient. Is that enough guidance for you to move forward?

[brettz9]

Ok, after reverting tests from using imports (and pointing to dist), reverting other ES6 features in test source, conditionally skipping one test which relied on Unicode RegExp flags, and using Mocha 3 (though without attempting nyc checks) and dropping babel/register for older Node support (FWIW, this is all in my test-old-node-dist branch), I've now confirmed that the mocha tests are passing on old versions of Node as far back as 0.10.0.

I was not able to successfully install earlier versions of Node (using nvm) to find out whether earlier versions might work.

So it seems pretty clear that his will continue to work on versions as far back as 0.10.0.

@mbroadst , do you have any objections to my keeping the addition of this PR to add engines with a minimum Node version of 0.10.0? It may technically be breaking, but since we can't verify it ever worked before 0.10.0, I'm not sure this should be such a big concern (and might be considered more of a "fix" since we are specifying what has been the expected behavior--that we only know it to work with 0.10.0+).

Btw, @michaelficarra , I've added a commit to drop testing previous to Node 6 (since it seems with the manual testing I've now done, you would like to keep the modern testing apparatus for the next (patch) release). And you can see that all automated tests are now passing as a result.

[brettz9]

One minor problem I have not been able to work around--though not sure we need to since npm test doesn't fail despite the 100% threshold--is that there is an apparent nyc bug (or @babel/register) whereby this PR causes nyc to report one line as not having an else condition covered, but the line in question (113 of esquery.js) is indeed being covered and has no else. Since this is not even a red if portion that is being reported, I think this very minor coverage regression is safe to ignore.

[mbroadst]

@brettz9 I think that's probably fine, thank you very much for your hard work!

[brettz9]

@michaelficarra In case you didn't get a notice, this should now be ready for review.

[michaelficarra]

How are we running the tests without building (npm run build) on this platform?

[michaelficarra]

Oh I see now. By not running the build step, we're testing against the old parser and will fail when we go to add a new feature.

[brettz9]

I don't recall if the build routine works or not (I can try for parity if you like), but it is not needed because the tests are able to run using @babel/register testing against the source instead of dist files.

(I only needed to switch to testing the dist files in the branch where I manually confirmed the old Node builds were still working. But that branch had to undo ESM in the test files and fixtures, avoid other ES6 features, as well as revert to the vulnerable Mocha 3.)

[michaelficarra]

I don't understand. Why would we be able to run "against the source" using @babel/register but not compile? Isn't @babel/register just compiling on the fly and running that?

[brettz9]

On the much older Node versions, @babel/register doesn't work (I switched those to avoid using babel register in favor of just testing against dist), and babel-register doesn't seem to be able to understand the import statements as @babel/register does.

As far as Node 6 that is failing on Travis, yes, @babel/register is compiling on the fly so the babel component should be doable, but it appears that the current @rollup/pluginutils (used by the current versions of @rollup/plugin-commonjs and @rollup/plugin-node-resolve needed in the Rollup config file) is using object spread properties which weren't supported in Node 6. Note that it wouldn't be enough to go back just to an older version here either, as these were previously not in the @rollup scope but were their own packages. But if we go too far back in our toolchain, then certain other features might not have been supported then. And even if we can use the older Rollup deps. across machines, then it may mean adding a version with vulnerabilities.

Is it really critical that the build scripts are confirmed to work on Node 6? These aren't available to work in the npm releases anyways (since the rollup config is not in package.json files and Rollup, etc. are devDeps).

This should really only be needed for those compiling from source on the repo who would presumably have access to more up to date machines. Seems it may be some unnecessary work for little benefit (not to mention adding to the travis build time).

But I can try to see if we can get it to work for Node 6 if it is important.

[michaelficarra]

This is good enough for now. We'll deal with node 6 CI issues if/when they come up in the future. Thanks for the hard work, @brettz9.

[brettz9]

Thanks for all the thoughtful reviewing and merges!