build: ditch transpilation

[gtramontina]

Addresses #217.

[codecov-io]

Codecov Report

Merging #218 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #218   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines          92     96    +4     
  Branches        4      4           
=====================================
+ Hits           92     96    +4

Impacted Files	Coverage Δ
lib/runner.js	100% <100%> (ø)	⬆️
lib/hook.template.js	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95f2f5e...6b9f0ff. Read the comment docs.

[ta2edchimp]

LGTM so far, just two things:

• Should we move the code from lib/install.js into bin (git mv and slightly alter the code to invoke the installHooks function immediately)? Might be only "cosmetical".

• Does the build prefix trigger a release (I'm genuinely not sure)? I think I would have chosen sth. like chore(build) which does not iirc.

[gtramontina]

👍 for moving install into bin. I think we can do this as a separate step (separate PR), as my initial intent with this one was to unblock #216.

Running npm run semantic-release locally, executes a dry-run. It gave me this back:

semantic-release WARN pre semantic-release didn’t run on Travis CI and therefore a new version won’t be published.
semantic-release WARN pre You can customize this behavior using "verifyConditions" plugins: git.io/sr-plugins
semantic-release ERR! pre Determined version 2.0.1 as "latest". Not publishing in debug mode. { type: 'patch', version: '2.0.1' }

Which apparently is correct (patch release: 2.0.1), or were you suggesting that there should be no release with these changes…?

[ta2edchimp]

I would interpret it as a breaking change to not support Node prior to v4 anymore (regardless of the reason behind), so I'm definitely against it being only a patch release.
So, if making a release, I think it should be a major version bump. And then we should make it properly, that is, removing the deprecation notice / update the readme and also move install to bin.

[gtramontina]

Oh yeah, definitely! Quit supporting node 4 is definitely a breaking change. I did think these changes would break compatibility, however everything is still fine with node 4 (and above):

• travis
• appveyor

Apparently we were only using babel for destructing a few requires, which I addressed like the following:

-const {resolve, basename} = require('path')
+const resolve = require('path').resolve
+const basename = require('path').basename

• lib/runner.js https://github.com/ghooks-org/ghooks/pull/218/files#diff-b3b53682a18f203ac8d29b0e277cad26L1
• lib/hook.template.js: https://github.com/ghooks-org/ghooks/pull/218/files#diff-84b93692b669eac624176e385c8ab3e9L2

[ta2edchimp]

So if it is a breaking change, it should be a major release then, shouldn't it?
And then, the deprecation status should be removed prior to releasing.

(Sorry for being so nitpicking 🙈)

[gtramontina]

No need to apologize! We're just trying to get on the same page… 😉

I do agree that if it is a breaking change, it should be a major release (as a general rule). Although that is not the case with the changes on this PR in particular. We didn't break compatibility with node 4.

Initially, I though we would have to stop supporting node 4, but it turns out it was very straightforward to remove babel from the game and keep the code compatible with node 4.

So, unless I'm not seeing something, we're good to tag these changes as patch… (?)

[ta2edchimp]

Let‘s do it 👍

Nevertheless, update the Readme / unmark as deprecated prior to releasing? Or do we wait until the first feature release?

[gtramontina]

Cool. Does that make sense? I feel like there's something iffy… (interpreting people's reaction from textual comms is hard) 😄

I guess we can do it (update readme and remove deprecation notice) as soon as 2.0.1 is out.

[ta2edchimp]

It’s all good. I‘m not a native english speaker, so things might sound different as intended.

I only worried about the package’s readme over at npmjs which will be the version at the time of publishing the release (that is, it might lead to reactions like „why releasing an update when it’s deprecated?“).
But I guess it’s a non-issue, since we want to get some more releases out soon.

So, from my end, let‘s go 🙂

[gtramontina]

I get you now… Over at npm it'll still read as deprecated. That's indeed a valid concern! How about this, then: I'll remove the deprecation note from the readme, add it as part of this PR and then we merge and call it 2.0.1.

ps: I'm not native english speaker either 😃

[ta2edchimp]

Awesome!