apiaryio / dredd

Language-agnostic HTTP API Testing Tool
https://dredd.org
MIT License
4.19k stars 279 forks source link

Remove proxyquire from the codebase #1058

Open honzajavorek opened 6 years ago

honzajavorek commented 6 years ago

proxyquire is a kind of hack and it only brings problems in long term, especially with the ES6+ import statement etc. We should use proper dependency injection or other techniques instead.


Note: This issue used to track licensing problems with proxyquire, but I updated it to track its removal instead.

realityking commented 6 years ago

PR to fix the licensing issue https://github.com/jbgutierrez/path-parse/pull/2

Looks like the package is essentially unmaintained, I doubt it'll ever get merged. Instead I took a look what it'd take to drop the dependency all together. Not looking to good either: https://github.com/browserify/resolve/issues/168

honzajavorek commented 6 years ago

Thanks for looking into this, @realityking. I think it should be possible to drop proxyquire from the codebase. It is there mostly as a relict of the CoffeeScript codebase and many old tests use it. I'd be happier if we could introduce Jest for mocking or just refactor the code so it's testable without tricks like proxyquire.

And yeah, Jest has also some licensing issues (mostly because of https://github.com/lerna/lerna/issues/1213), but I'm keen to work on those (which I do https://github.com/lerna/lerna/pull/1441, https://github.com/lerna/lerna/pull/1465) instead of sticking to proxyquire.

realityking commented 6 years ago

For tests yes, but proxyquire is also used to set up the hook files: https://github.com/apiaryio/dredd/blob/5cd3529a612913e72da71d8fa2d5ee330fb4eafb/src/add-hooks.js#L6

honzajavorek commented 6 years ago

Oh, it injects the hooks. Hmm. Wondering whether there is an alternative solution to that 🤔

realityking commented 6 years ago

I see two options:

  1. Simply make hooks available as a global in the execution context.
  2. Inject a global variable like above with the actual logic but expose an entry point dredd/hooks that contains the definitions of all functions. This would have the sweet advantage of giving us working autocomplete in IDEs.
  3. Like in the sandboxed version, directly expose beforeEach, afterAll, etc. like in the sandboxed hooks.

I like option 2 and would be happy to test any of them out (tell me which one you prefer!) but it'd be a breaking change.

honzajavorek commented 6 years ago

I like the option 2! We could do this as a separate change, or in the wider context of JS hooks revamp. The current implementation has many issues. If you found any of those worth your time, it would be totally awesome. I'm definitely eager to have breaking changes if they improve the software and people's lives 😄

Just for the sake of consistency, let's see how other languages do it:

I think having dredd/hooks pretty much fits in.

honzajavorek commented 6 years ago

@realityking I can upgrade to the latest proxyquire now. I'll reform this issue though into one tracking we should remove proxyquire in the future. I think we had a valuable discussion about the topic here.