Rewrites Gavel core to JavaScript #132
Conversation
5ad5fdb
to
2e34392
Compare
0f1b645
to
25bf9e2
Compare
|
Experiencing failure of CLI behavior tests on CI only. Passes on local. |
bf86f2d
to
55d0ea6
Compare
|
Current CI fails on Line 21 in 755e567 When copied, test modules are one level deeper than the original location. This breaks modules resolution when requiring source modules of Gavel. |
e19c822
to
472acc1
Compare
472acc1
to
5e4441a
Compare
|
After investing a few hours into test coverage script issue, I've managed it to work, but 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. |
| "coverage": "scripts/cov", | ||
| "coveralls": "npm run coverage mocha-lcov-reporter | coveralls", | ||
| "ci:lint": "npm run lint", | ||
| "ci:test": "npm run test && npm run coveralls", |
There was a problem hiding this comment.
Temporary removed. Will be introduced back as a part of the next pull request.
There was a problem hiding this comment.
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 :).
There was a problem hiding this comment.
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 :)
src/model/http-request.js
Outdated
| } | ||
| } | ||
|
|
||
| HttpRequest.resourceKeys = ['method', 'uri', 'headers', 'body', 'expected']; |
There was a problem hiding this comment.
Setting on a class itself, since this.resourceKeys fails gavel-spec assertion of how classes should look, while static resourceKeys is not commonjs syntax.
There was a problem hiding this comment.
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.
| "coverage": "scripts/cov", | ||
| "coveralls": "npm run coverage mocha-lcov-reporter | coveralls", | ||
| "ci:lint": "npm run lint", | ||
| "ci:test": "npm run test && npm run coveralls", |
There was a problem hiding this comment.
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 :).
|
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. |
There was a problem hiding this comment.
Approving, since I see the comments addressed and this is going to a dedicated branch, not into master. Thanks @kylef for the thorough review 🙏
|
We'll address the browser tests in a separate PR to the dedicated branch. |
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
libdirectory and any pointers to it, since no compilation steps is needed anymore (no.coffee->.jsbuild).GitHub
Roadmap