New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weโ€™ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove automated compiling of CoffeeScript #1082

Merged
merged 1 commit into from Jan 14, 2019

Conversation

Projects
None yet
3 participants
@realityking
Copy link
Contributor

realityking commented Jul 8, 2018

๐Ÿš€ Why this change?

This change removes automated compiling of CoffeeScript and instead lets users add any kind of compiler, this would, for example, be useful to support Babel. It also remove the dependency on coffeescript which is nice for us non-coffeescript users :)

This will require a major version release.

๐Ÿ“ Related issues and Pull Requests

โœ… What didn't I forget?

  • To write docs
  • To write tests
  • To put Conventional Changelog prefixes in front of all my commits and run npm run lint

I didn't add any new tests as the feature is covered with existing tests that use coffeescript.

@realityking realityking force-pushed the realityking:no-coffeescript branch 2 times, most recently from f36b31b to c466c22 Jul 8, 2018

@honzajavorek

This comment has been minimized.

Copy link
Member

honzajavorek commented Jul 17, 2018

It might take me a while to review and think through all the consequences, but I love this! One thing which is sure is this needs a BREAKING CHANGE: section in the commit as it is a breaking change for anyone using the CoffeeScript hooks.

@honzajavorek

This comment has been minimized.

Copy link
Member

honzajavorek commented Jul 17, 2018

I didn't add any new tests as the feature is covered with existing tests that use coffeescript.

Are there dedicated tests to verify hooks work with CoffeeScript as such, or do we rely on the rest of the tests using not-yet-decaffeinated hooks?

Since we have both CS and Babel in the (dev)deps, it would be nice to have two dedicated e2e test cases to be sure at least those two compilers are supported and tested.

@realityking

This comment has been minimized.

Copy link
Contributor

realityking commented Jul 17, 2018

I figured this would take a while to merge because it does make sense to batch it with some other breaking changes.

Currently there's no dedicated test to cover this functionality, I'll add some. (Might be a few days until I get to this)

@honzajavorek
Copy link
Member

honzajavorek left a comment

Okay, I could not hold myself and I did a full review after all ๐Ÿ˜„ See my comments, please.

One more thought: Will this work for the sandboxed hooks as well? I have no intention to keep those working with CoffeeScript out of the box, but if we're doing this, we should support it (and test it) for sandboxed hooks as well.

@@ -71,6 +70,7 @@
"babel-core": "6.26.0",
"babel-preset-env": "1.6.1",
"body-parser": "1.18.3",
"coffeescript": "1.12.7",

This comment has been minimized.

@honzajavorek

honzajavorek Jul 17, 2018

Member

Once we have it in the tool chain only to verify --require=coffeescript/register works, we could actually upgrade it to 2.x to get rid of the annoying warnings. But that's just a note for future from the top of my head, nothing to care about now.

This comment has been minimized.

@realityking

realityking Jul 17, 2018

Contributor

Yeah I thought the same but I think this is better left for a separate PR.

}
// eslint-disable-next-line global-require, import/no-dynamic-require
require(mod);
}

This comment has been minimized.

@honzajavorek

honzajavorek Jul 17, 2018

Member

Can we test and ensure whether Dredd is able to gracefully handle errors here?

  • Dredd aborting with a sane message in case the path is wrong
  • Dredd aborting with a sane massage in case the required module blows up (at least try/catch synchronously or something)
@honzajavorek

This comment has been minimized.

Copy link
Member

honzajavorek commented Jul 17, 2018

I figured this would take a while to merge because it does make sense to batch it with some other breaking changes.

I don't see an issue with that. I'm fan of continuous delivery. When debugging, it's easier to identify what change caused the problems, for both the maintainers and the users (they can try granular versions and say which is the last one working and it's clear what change the version brings).

Just add the BREAKING CHANGE: section to the end of the commit, with an explanation and possibly a migration hint. Once it's merged, it gets autoreleased and I'm totally fine with major bump.

Might be a few days until I get to this

No problem, it took me quite a few days to get to reviewing this gem ๐Ÿ’Ž, so I should be apologizing mysefl. Let's skip apologies and make awesome stuff when we have the time ๐Ÿ˜‰

@realityking realityking force-pushed the realityking:no-coffeescript branch from c466c22 to e5a0f47 Aug 12, 2018

@realityking

This comment has been minimized.

Copy link
Contributor

realityking commented Aug 12, 2018

Changes I've made:

  • Added error handling
  • Test for coffeescript
  • Test for babel-register
  • test for error case
  • added breaking change to the commit

This leaves the sandbox case, I need to play with that and understand how it works.

@realityking

This comment has been minimized.

Copy link
Contributor

realityking commented Aug 12, 2018

Hmm, tests are failing on Travis but working locally. Any idea what would cause this?

@honzajavorek

This comment has been minimized.

Copy link
Member

honzajavorek commented Aug 13, 2018

I see some linter errors, but I guess that's not the real issue. Both AppVeyor and Travis agree there's a problem with

  Specifying neither response body nor schema in the API Blueprint
    when the server returns non-empty responses
TypeError: Cannot read property 'code' of null
    at runDredd (/home/travis/build/apiaryio/dredd/test/integration/require-test.js:67:28)
    at recordLogging (/home/travis/build/apiaryio/dredd/test/integration/helpers.js:141:7)
    at fn (/home/travis/build/apiaryio/dredd/test/integration/helpers.js:35:5)
    at configuration.emitter.emit (/home/travis/build/apiaryio/dredd/src/dredd.js:29:3501)

on multiple Node versions. Doesn't look flaky to me. Let me see if I can find a while to reproduce or debug this. For now I'd focus on looking for discrepancies between your local setup and the CI setup.

@realityking realityking force-pushed the realityking:no-coffeescript branch from e5a0f47 to d71eb4a Aug 27, 2018

@realityking

This comment has been minimized.

Copy link
Contributor

realityking commented Aug 27, 2018

I discovered that the difference is in whether the test suite is called directly (npm run test) or with coverage (npm run test:coverage).

I tried the naive thing of replacing Istanbul with nyc, but that just got me a new set of errors.

I think this is the point where I have to give up, the test suite (and dredd itself) are too complex. I'll leave this open in case you wanna take a stab at it, otherwise feel free to close it.

@honzajavorek

This comment has been minimized.

Copy link
Member

honzajavorek commented Aug 28, 2018

Iโ€™m sad to read that, but I suppose itโ€™s true. The tests are way too complex sometimes, especially when taken as the whole suite.

Iโ€™ll definitely leave this open and hopefully it gets into my pipeline soon.

@honzajavorek

This comment has been minimized.

Copy link
Member

honzajavorek commented Dec 27, 2018

I'd like to revive this soon. I know I neglected it for quite some time, so I'm ready to take over the responsibility of rebasing etc., but any help is appreciated ๐Ÿ™

@realityking realityking force-pushed the realityking:no-coffeescript branch 2 times, most recently from b700f3c to b3a9ffb Jan 9, 2019

@realityking

This comment has been minimized.

Copy link
Contributor

realityking commented Jan 9, 2019

I took care of the rebasing and new eslint errors, I hope that makes things a little easer for you. The error from #1082 (comment) is still present.

@realityking realityking force-pushed the realityking:no-coffeescript branch from 2f042eb to 7cd3bec Jan 9, 2019

@realityking

This comment has been minimized.

Copy link
Contributor

realityking commented Jan 9, 2019

Looks like I actually got it work ๐Ÿ˜ณ

The errors now are only from coveralls. I'll see sometime later this week if things still work when I go back to Istanbul, otherwise I'll see what's up with nyc.

@honzajavorek
Copy link
Member

honzajavorek left a comment

The rebase had to be brutal ๐Ÿ™ˆ Thank you so much. I posted some suggestions for the docs.

Show resolved Hide resolved docs/hooks/js.rst Outdated
Show resolved Hide resolved docs/hooks/js.rst Outdated
@honzajavorek

This comment has been minimized.

Copy link
Member

honzajavorek commented Jan 10, 2019

I'm actually wondering whether the current setup with Coveralls makes any sense. Coveralls don't report back under PRs and I wasn't able to debug it. Without that integration, it's just a useless number on a README badge, which requires a lot of complexity in how we run tests.

Also, after discussing the usefulness of coverage with @smizell on Twitter I have heretic thoughts about removing coverage altogether...

@realityking

This comment has been minimized.

Copy link
Contributor

realityking commented Jan 10, 2019

I pretty much rewrote half the changes but it wasn't too bad ๐Ÿ˜†

As for coverage, if you wanna keep it around, I had a much better time with codecov than with coveralls.

@realityking realityking force-pushed the realityking:no-coffeescript branch from e8d534b to 99c96cb Jan 10, 2019

@honzajavorek

This comment has been minimized.

Copy link
Member

honzajavorek commented Jan 10, 2019

I think I've considered these as alternatives in the past, but didn't finish the evaluation:

@realityking

This comment has been minimized.

Copy link
Contributor

realityking commented Jan 10, 2019

Itโ€™s green ๐Ÿ˜ญ๐Ÿฅณ

That leaves the problem of sandboxes hooks. I saw the proposal to simple rip them out (#1178). Is that an option?

@honzajavorek

This comment has been minimized.

Copy link
Member

honzajavorek commented Jan 10, 2019

๐Ÿ‘๐Ÿ‘

Already in action: d0a2957

@realityking

This comment has been minimized.

Copy link
Contributor

realityking commented Jan 10, 2019

Perfect!

@honzajavorek

This comment has been minimized.

Copy link
Member

honzajavorek commented Jan 11, 2019

Here it is: #1196

@realityking realityking referenced this pull request Jan 11, 2019

Merged

Remove sandboxed hooks and old APIs #1196

3 of 3 tasks complete
@realityking

This comment has been minimized.

Copy link
Contributor

realityking commented Jan 12, 2019

Curios, why didnโ€˜t you make this PR part of 6.0?

@honzajavorek

This comment has been minimized.

Copy link
Member

honzajavorek commented Jan 12, 2019

I forgot itโ€™s breaking change as well ๐Ÿคฆโ€โ™‚๏ธ

Let's do it as a next step then. We can rebase on top of current master and make another release, Iโ€™m not against it.

Actually, I should drop the โ€œmajor release ceremonyโ€ sentiment altogether. With a release for every change people will have version granularity when upgrading and itโ€™s easier for them to debug problems on the way.

Iโ€™ll release this early next week.

feat: remove automated compilation of CoffeeScript
Instead adding a new option "require" to enable users to still use CoffeeScript

BREAKING CHANGE: Hookfiles using CoffeeScript are not supported out of the box anymore. Instead manually install the coffeescript module and add --require=coffeescript/register to your command.

@realityking realityking force-pushed the realityking:no-coffeescript branch from 99c96cb to 712a2e4 Jan 12, 2019

@realityking

This comment has been minimized.

Copy link
Contributor

realityking commented Jan 12, 2019

Rebased on top of master.

@honzajavorek honzajavorek merged commit e963159 into apiaryio:master Jan 14, 2019

4 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - docs/requirements.txt (Apiary) No manifest changes detected
security/snyk - package.json (Apiary) No new issues
Details
@ApiaryBot

This comment has been minimized.

Copy link
Collaborator

ApiaryBot commented Jan 14, 2019

๐ŸŽ‰ This PR is included in version 7.0.0 ๐ŸŽ‰

The release is available on:

Your semantic-release bot ๐Ÿ“ฆ๐Ÿš€

@ApiaryBot ApiaryBot added the released label Jan 14, 2019

honzajavorek added a commit that referenced this pull request Jan 14, 2019

kylef added a commit that referenced this pull request Jan 14, 2019

@realityking realityking deleted the realityking:no-coffeescript branch Jan 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment