Rewrites Gavel core to JavaScript

[artem-zakharchenko]

Description

This pull request aims to rewrite Gavel to JavaScript, replacing CoffeeScript. It doesn't introduce any logic or API changes, and it doesn't change tests (still written in CoffeeScript), to retain minimal surface of changes.

Subsequent pull requests will follow to rewrite test suits in JavaScript, and improve internal logic, introducing breaking changes.

Change log

1. Rewrites Gavel's source to JavaScript from CoffeeScript.
2. Removes lib directory and any pointers to it, since no compilation steps is needed anymore (no .coffee -> .js build).
3. Temporary removes test coverage collector. To be reverted in the next pull request, once test suits are rewritten to JavaScript.

GitHub

• Relates to Decaffeinate the codebase #96

Roadmap

• Mixins
  • validate-http-message
• model
  • http-request
  • http-response
• utils
  • extendable
  • get-type
  • schema-v4-generator
  • tv4-to-headers-message
• validators
  • headers-json-example
  • json-example
  • json-schema
  • text-diff
  • validation-errors
• errors
• gavel
• meta-schema-v3
• meta-schema-v4
• validate
• validators
• Ensure cucumber tests pass
• Ensure Gavel is compiled to ES5

[artem-zakharchenko]

Experiencing failure of CLI behavior tests on CI only. Passes on local.

[artem-zakharchenko]

Current CI fails on scripts/cov trying to collect test coverage. The problem is here:

gavel.js/scripts/cov

Line 21 in 755e567

cp -r ./test ./src-cov/test

When copied, test modules are one level deeper than the original location. This breaks modules resolution when requiring source modules of Gavel.

[artem-zakharchenko]

After investing a few hours into test coverage script issue, I've managed it to work, but mocha-lcov-reporter still screams that there is no test coverage collected.

The script itself is needed only to collect coverage from tests written CoffeeScript. Since the next step of this migration is to rewrite tests to JavaScript, we can easily collect coverage from JavaScript tests without having to have a custom script.

I'm removing coverage collection as of now, and changing the target branch of this pull request to an intermediate branch that would contain the entire rewrite to JavaScript.

[artem-zakharchenko]

Temporary removed. Will be introduced back as a part of the next pull request.

[kylef]

Make's sense, I think it's usually encouraged to create a GitHub issue to track that. Intentions may be that you'll work on it next but then something else may come up etc and this task can get easily forgotten.

I was trying to find some wise words from @honzajavorek (in a comment) where he told me the same thing while reviewing an OAS 3 parser change, I think I still didn't get to solve whatever it was :).

[artem-zakharchenko]

Mentioned here: #96 (comment)

It's a part of decaffeination, so it's under the respective issue. As I've mentioned, this branch is not going to master, it's but a step in this venture :)

[artem-zakharchenko]

Setting on a class itself, since this.resourceKeys fails gavel-spec assertion of how classes should look, while static resourceKeys is not commonjs syntax.

[kylef]

My biggest concern with these changes is that this breaks support for some browsers. Before, we are compiling coffeescript into ES5, the code should work on older browsers (I can see there is some form of browser tests in package.json so it seems the intention is this library should work in browsers) "test:browser": "mochify \"test/unit/**/*.coffee\" --transform=coffeeify --extension=.coffee",.

With this change we will now distribute ES6 only sources which won't work in all browsers. I am not sure, but I will make the assumption that gavel is used in the browser by Apiary and this may cause a regression in browser support.

If this is intended, then perhaps a commit message with BREAKING CHANGE: is nessecery to indicate that the next version is major (https://www.conventionalcommits.org/en/v1.0.0-beta.3/#commit-message-with-description-and-breaking-change-in-body) as this library uses semantic releases. Perhaps it would be better to add babel or similiar to create ES5 sources for the browser (for example: refractproject/minim#209).

---

I've made various types of comments so I will explain them here.

• Style guide -- Spotted code that doesn't follow style guide, to be honest I am not sure I'd block PR on this and I think integrating our style guide and eslint could be a separate changeset.
• Previous behaviour -- Sometimes I am not sure that the existing behaviour is correct, these times I've usually cc-ed HJ so he can read these when he gets back and see if his domain-knowledge here can explain them or if we should create separate issues to investigate / resolve potential bugs. One example is creating JSON Schema with "empty" property, I am not sure this exists in JSON Schema draft 4
• Changes in functionality -- Possible changes in functionality, we can lean on the test coverage but I may have found some things that I think may be altered functionality which can be missing test coverage for certain inputs.

[kylef]

Make's sense, I think it's usually encouraged to create a GitHub issue to track that. Intentions may be that you'll work on it next but then something else may come up etc and this task can get easily forgotten.

I was trying to find some wise words from @honzajavorek (in a comment) where he told me the same thing while reviewing an OAS 3 parser change, I think I still didn't get to solve whatever it was :).

[artem-zakharchenko]

The next necessary steps are described here #96 (comment).

I wouldn't create separate issues because it can't be worked in parallel. That would only confuse, as all these must be performed in a sequence.

[honzajavorek]

Approving, since I see the comments addressed and this is going to a dedicated branch, not into master. Thanks @kylef for the thorough review 🙏

[honzajavorek]

We'll address the browser tests in a separate PR to the dedicated branch.