Remove direct dependency on Sinon

[IanVS]

Why are Sinon methods being exported from this package (for example in https://github.com/airbnb/reagent/blob/master/src/index.js#L20)? This poses a problem for us, as Sinon is throwing an exception on our codebase and we don't necessarily want to use Sinon and would rather have the flexibility to choose our own mocking/stubbing library. I understand that it's perhaps a convenience to be able to useSinon, but It seems outside of the scope of what Reagent is trying to accomplish, and it is trivial to set up Sinon ourselves. Is there a chance you'll rethink this approach? I'd really like to be able to use Reagent for testing, but right now this is preventing us from doing so.

[ljharb]

It seems reasonable to move the sinon stuff into a reagent-sinon package which you could use if you wanted that behavior.

[mudetroit]

I think it makes a ton of sense to not have sinon as part of the main package. I am curious what direct benefit a user of reagent would get from including sinon with reagent that wouldn't be there if you depended on sinon directly.

Additionally the current implementation makes assumptions about test framework lifecycle methods that might not make sense in every environment.

[ljharb]

I'm not sure what assumptions you're talking about - could you please file a separate issue for that?

[IanVS]

Not every test framework has a beforeEach and afterEach.

[mudetroit]

@ljharb not certain it is worth an issue. but before and after while available in most frameworks aren't present in them all. Qunit for example uses setup and teardown.

[lelandrichardson]

The use of sinon in this project has been largely left undocumented because of the assumptions it's making about mocha being present (which is a vestige of this being used internally at airbnb).

I agree this should be done as it is somewhat separate from the meat of the library.

[just-boris]

+1
I am using this with jasmine, and I don't need an extra library for spies, since I have a built-in one.

[Sinewyk]

Considering the related PR was just closed with no comments, what does this mean for this issue ?

Do you still accept work for this or do you have other plans ?

[lelandrichardson]

@Sinewyk this is still going to happen! I just closed the PR because this branch was doing some mono-repo things that we've decided not to do.

This is definitely planned for 2.0!

[mikl]

Just fyi., the Sinon dependency could also be a significant blocker to adoption, since Sinon has issues with WebPack, so it starts puking out errors as soon as I add a import { shallow } from 'enzyme' to my project.

WARNING in ./~/sinon/lib/sinon.js
Critical dependencies:
40:25-32 require function is used in a way in which dependencies cannot be statically extracted
 @ ./~/sinon/lib/sinon.js 40:25-32

I'm using a setup based on the react-redux-starter-kit, but there's plenty of other people with the same problem, and the accepted workaround won't work here, since I can't modify how Enzyme requires Sinon (without forking).

[lelandrichardson]

@mikl I totally 100% agree. Enzyme 2.0 will be coming out soon with this as one of the breaking changes. You can try pulling in the RC in the time being (no guarantees on what that will do though):

npm i enzyme@2.0.0-rc1

Part of enzyme 2.0 will be that all major test environments will be supported, and this will be ensured by having "example" projects whose test suites actually run as part of the CI of this project.

Sorry again for the frustrations that this is causing at the moment.

[IanVS]

@lelandrichardson no need to apologize! You're doing us all an awesome service by open sourcing this. Thanks for all your hard work on it.

[mikl]

@lelandrichardson thanks, no need to apologise. It 2.0.0-rc1 solves the Sinon problem, although I still needed one of the workarounds from #47 to make it all work.

[lelandrichardson]

@mikl yes - some of the problems present in #47 and other issues are not easy problems to fix as they are present in part because there are conditional require statements that are different for react 0.13 and react 0.14, which could only be fixed through dropping 0.13 support. Additionally cheerio has some problems specifically which would mean we'd have to drop render support or roll our own cheerio.

With the release of 2.0 I will include guides for the steps to get enzyme working in every popular testing environment that I'm hoping will achieve a good balance of ease-of-installation and compatibility.

[mikl]

@lelandrichardson yeah, nothing like multi-version, multi-environment support to create work for you 😞. Thanks for your efforts.