Add support for TypeScript

[lddubeau]

The main point of this branch is to add support for TypeScript.

There are two other changes on this branch in separate commits so that things can be cherry-picked if needed:

• I've removed the trailing whitespaces in patterns.js
• I've added an .editorconfig. It was automatically generated from the code already present. I eyeballed it and did not see any problems.

I've just noticed the tests failed on Node 0.8 and 0.6. Maybe it is time to stop supporting them?

[lddubeau]

I needed TypeScript support to be present in a released tool urgently. After a week without any feedback I've decided to fork.

You can still merge the pull request if you want. Or close it, if TypeScript support is beyond the scope of what you think semver-sync should do.

[cimi]

Apologies for the silence - I've been on holiday and then pretty busy in the past weeks.

You've contributed a lot to this library and I am really grateful for that. I am using it in a few small projects but it does all I need so I won't be investing any significant amount of time in it soon. If you want to take the project further I am happy to transfer ownership to you (although I'm not sure how that works on npm, haven't done it so far).

You do have write access to this repo (or should have, I remember granting that a while ago); if you want to contribute code feel free to do so without asking me, as long as the tests pass and you are not fundamentally changing existing behaviour without discussion.

Let me know how you'd like to proceed in the future, I am fine with both transferring ownership to you or you committing here without asking me.

[lddubeau]

I'm glad you were on vacation and not the victim of a disaster.

Yes, I do have write permission on this repo. This pull request was actually pushed as a branch in this repo, but...

as long as the tests pass and you are not fundamentally changing existing behaviour without discussion.

That's the thing. Adding TypeScript could be considered significant enough to require discussion. Or perhaps not, but removing support for Node 0.8 and 0.6 is a breaking change. (I did not investigate thoroughly why the tests no longer passed on 0.8 and 0.6 but I suspect it is simply because the typescript package won't run on them. So it looks like supporting TypeScript entails discontinuing support for 0.8 and 0.6.)

And the tests were not passing, due to failures on 0.8 and 0.6. So it fails the "as long as the tests pass" criterion, and I saw the PR as requiring discussion.

However, there's also the issue of my longer term plans which are even more radical than just adding TypeScript support. For now, I've released a version 2.0 of the fork I made which is pretty much what I had submitted here, plus a few small modifications. But then I've started working on the next release. One thing that I've been wishing for is the ability to perform the actions that semver-sync allows without having my build tools run semver-sync as if I were typing it at the shell. In other words, replace this:

gulp.task("semver", () => execFileAsync("semver-sync", ["-v"]));

(execFileAsync is child-process's execFile wrapped by Bluebird to return promises.)

with something like this:

import semverSync from "semver-sync";

gulp.task("semver", () => semverSync.run({ verify: true }));

Where the .run method is part of an official API. As part of this change, what is exported by index.js has changed a lot. For instance, almost all functions return promises rather than rely on callback or use the sync version of Node's library calls. Anybody who ever relied on using directly what index.js exports won't be able to do so with the next version.

This may be more radical than what you want for semver-sync.

[cimi]

I'm glad you were on vacation and not the victim of a disaster.

Haha, far from it. :) I went to this which was awesome.

That's the thing. Adding TypeScript could be considered significant enough to require discussion. Or perhaps not, but removing support for Node 0.8 and 0.6 is a breaking change.

Both versions have been end-of-life'd a long time ago. We could just do a major version bump and only test against supported node versions (which is what I suppose you did for 2.0). People stuck on older versions could use the old version of semver-sync.

One thing that I've been wishing for is the ability to perform the actions that semver-sync allows without having my build tools run semver-sync as if I were typing it at the shell.

Having full programmatic access without having to fall back to the shell sounds perfectly sensible to me. I've not documented any of the features in index.js so anybody relying on that would have assumed a certain level of risk anyway. As long as we're careful about respecting semantic versioning and release a new major version when we introduce breaking changes I think it's good for the package to evolve.

[lddubeau]

Alright, maybe I can just incorporate everything here then.

A couple additional potential sticking points:

1. I've moved the test suite to Mocha. Reasons:

   a. I'm very familiar with Mocha, but not so much with tap.

   b. I've read this, so I thought I'd give tap a fair shake and spent time actually implementing a versync suite both in Mocha and tap/tape. The Mocha suite was faster and there's none of the supposed downsides of Mocha that I found problematic.

   c. It does not appear possible with tap or tape to narrow a test run to a single test merely by passing a command line argument or configuration option. It is possible to use an .only method to select a single top level test but if you have tests in tests, which is pretty much bound to happen and want to select a nested test, forget it.

   This is an architectural limitation. Mocha works in a way that allows it to know ahead of time all the tests that exist in the suite, whereas tap and its derivates don't know whether a top-level test will also contain a test until they actually run the top-level test.

   I use Mocha's capability to run only selected tests a lot so not having this capability in a test runner is a major issue for me.

   This being said, there's one thing I wish Mocha had and that I think is a significant gap in Mocha's capabilities: assertion counting.

2. I've deprecated using commas in versionedSources, with support to be ultimately removed in a future (yet undetermined) version. I believe semver-sync currently chokes if versionedSources is an array. The post-2.0 code supports arrays, which makes the need for comma-separated values unnecessary. (If there's some substantial reason commas should be supported for ever and ever, I could go back on this.)

I've also made everything pass eslint checks, which currently are set to my preferences but that could be adjusted so this is probably not a sticking point. (I use the config published by Airbnb + some of my own preferences.)

[cimi]

I've moved the test suite to Mocha

No problem here. I've started using Mocha as well in the past year and I like it a lot. I know what you mean about running tests selectively, it's really useful. This is the one and only project I've used tap in so I'm not emotionally attached.

I've deprecated using commas in versionedSources, with support to be ultimately removed in a future (yet undetermined) version. I believe semver-sync currently chokes if versionedSources is an array. The post-2.0 code supports arrays, which makes the need for comma-separated values unnecessary. (If there's some substantial reason commas should be supported for ever and ever, I could go back on this.)

Not sure I understand here. Using arrays instead of parsing strings makes sense and it's probably what I should've done from the beginning. Not sure what you mean by 'deprecated commas' though. Do you issue a warning?

I've also made everything pass eslint checks, which currently are set to my preferences but that could be adjusted so this is probably not a sticking point. (I use the config published by Airbnb + some of my own preferences.)

That's great. I'm not fussy about styling rules as long as everything is consistent. If you can provide an .eslintrc so that it's formalised and can be picked up by tooling (i.e. SublimeLinter) I'm fine with it.

Let me know if you'd like me to review any code, I have some time available today and tomorrow.

[lddubeau]

Sorry, I've been cryptic there. semver-sync accepts this:

"versionedSources": "foo.js, bar.js"

It will split the value on the commas and take foo.js and bar.js as the versioned files. That's deprecated in favor of:

"versionedSources": ["foo.js", "bar.js"]

For now, you get a warning if you use a value that is a string that contains a comma.

It will take a bit before I can put something up for review. In the process of working on the code, I found that chai-as-promised has a significant problem. I'm still working on getting that fixed.

[lddubeau]

Now that I've submitted the PR for chai-as-promised, I can think about this again. I've pushed the work in progress here. I'm still in the midst of fleshing out the test suite, and I've not updated the README to point to the API.

I noticed one major thing I did not mention is that this work in progress drops support for Node 0.10 too because it makes use of features of ES6 that are supported natively only in Node 4 and over: const, let, arrow functions, class, backtick strings, and some other minor things.

LTS support for 0.10 is ending on October 1st so I had decided that it was not worth having the radical rework I was engaged in be written to support 0.10.

[lddubeau]

I've just pushed what could be called the "release candidate" for 3.0.0.

One new feature: support for ES6. This entailed switching from UglifyJS to esprima to do the JavaScript parsing. UglifyJS has no official support for ES6. I had not realized UglifyJS did not support ES6 but I had the surprise when I tried running versync on its own code and it did not work. Oops...

I noticed the test suite fails on Travis, even though it runs fine here. I see there's a linting problem (probably some peer dependency not being satisfied) and I suspect I oversimplified the git tests. I'll get around to fixing that before a release is made.

So what's the way forward? I'm okay with transferring ownership to me. It would have to be done both on github and npm. Or I could merge everything back here, but I'd still need to be co-owner on npm to be able to publish.

[cimi]

So what's the way forward? I'm okay with transferring ownership to me. It would have to be done both on github and npm. Or I could merge everything back here, but I'd still need to be co-owner on npm to be able to publish.

It's literally your call - both options work for me. I can also mark semver-sync as deprecated and point to your versync. I don't mind either way, but I feel you should get the credit for the complete overhaul.

I'm happy to review the code if you want me to, but I can't commit to getting it done this week as I'm full at work and I'm going to a hackathon over the weekend. I will try and do it as soon as I can, if I don't manage this week I will reserve time early next week.

[lddubeau]

I think the simplest solution is to mark semver-sync as deprecated and point to versync.

I'll take you up on the code review. Just let me know if something comes up that would significantly push back the review.

There's a very remote chance that circumstances could push me to release 3.0.0 before the time frame you mentioned but as I said, that's very remote.

I'll open a formal pull request on the versync side which will allow for discussion of the proposed 3.0.0 there.