Skip to content
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

Sinon 2.0 #600

Closed
wants to merge 9 commits into from
Closed

Sinon 2.0 #600

wants to merge 9 commits into from

Conversation

cjohansen
Copy link
Contributor

Making this a pull request that can be used to track progress and dependencies.

  • Build with browserify
  • Run browserified bundle in browsers
  • Extract fake server
  • Fix deferred tests
  • Run CI tests against both source and built version(s)
  • Lint
  • Figure out a solution for overridable sinon.log
  • Figure out a solution for overridable sinon.format
  • Figure out an alternative for sinon.config
  • Figure which parts of the public API no longer needs to be public (extend, etc)
  • Figure out which parts to extract as optional add-ons (perhaps assert, test)
  • Use this as little as possible
  • Get rid of the sinon.spy() / sinon.spy.create() duality. We don't need sinon.spy.create and the like
  • Do we still want to support mocks? Are people using mocks?
  • Clean up code organization: stream-line file names, flatten directory structure
  • Upgrade Buster to a more recent version
  • Include https://github.com/mroderick/sinon-555 as an automated test somehow
  • Stubbing an object should not execute getters (in supported browsers)
  • Stubbing an object property should fail if it is a getter (in supported browsers)
  • Make fake XHR/server load on Node - remove the checks

This is the first bold step towards getting rid of all kinds of annoying
packaging troubles with Sinon.JS. This allows the source code to be
developed as a regular node.js module, and eventually package everything
up in a umd-style wrapper for the browser using browserify.

In this first step there are several things that need attention:

1. I have not attempted to run this in a browser, or even build it with
   browserify
2. Because of 1, the fake server stuff has not yet been considered
3. Some tests have been deferred for now
4. Features that are based on the user overwriting properties on the
   sinon object does not work correctly: sinon.log, sinon.format,
   sinon.config. We need to discuss how to solve them. I suspect we will
   not be able to ship API-compatible versions of these (in the case of
   sinon.config, I wouldn't really want to either, I always hated that
   "API")
5. The code-base must be re-linted with "use strict" settings

Next steps:

* Lint
* Extract the fake server stuff into a separate module
* Figure out a solution for overridable sinon.log
* Figure out a solution for overridable sinon.format
* Figure out an alternative for sinon.config
* Figure which parts of the public API no longer needs to be
  public (extend, etc)
* Figure out which parts to extract as optional add-ons (perhaps assert,
  test)

Eventually I also want to clean up some stuff for the 2.0 release:

* As little use of `this` as possible. Currently, there's quite a bit
* Get rid of the sinon.spy() / sinon.spy.create() duality. We don't need
  sinon.spy.create and the like
* Do we still want to support mocks? Are people using mocks?
* Clean up code organization: stream-line file names, flatten directory
  structure
* Upgrade Buster to a more recent version

...and I'm sure there's tons more to do. This is a start.

Ping @mantoni, @mroderick
@mroderick
Copy link
Member

Use this as little as possible is a hard to measure goal. Perhaps we can come up with a number of recipes for avoiding the use of this and then use those when reviewing code?

@cjohansen
Copy link
Contributor Author

I agree, it's a hard goal to quantify. What I mean is that I want none of Sinon's stuff to depend on this, so people can ship functions around and compose them at will without involving bind or other workarounds.

@mroderick mroderick added this to the 2.0 milestone Nov 13, 2014
@mroderick
Copy link
Member

Can sinon-2.0 branch be the canonical branch for the 2.0 work, and we can make pull requests into it and not into master?

I've created a 2.0 milestone, please add your pull requests to that.

Issues for 2.x should be tagged with the 2.x tag.

@cjohansen
Copy link
Contributor Author

Sounds like a plan! We need to build on top of this anyway - the moment we allow development to diverge from this work now, it will most likely be futile, as merging with this from something based on master will be difficult to say the least. This should be merged back in some reasonable time, within 2014 at least.

@mantoni
Copy link
Member

mantoni commented Nov 13, 2014

This is all really great stuff. I'll see if I can free up some time to help working on this.

Regarding Buster @cjohansen: You said you didn't try to run the tests in a browser. Can Buster be Browserified now? When I looked at running lolex with Buster in browsers, it didn't work. We ended up using Mochify and migrating all the test cases to Mocha.

@cjohansen
Copy link
Contributor Author

@mantoni ah, so that was the reason. I couldn't remember. My plan at this point is to build a sinon bundle with browserify and test with buster loaded on the page. Nothing's set in stone.

@GCheung55
Copy link
Contributor

I can think of four possible ways of loading Sinon.JS in the browser.

  1. Load each file in order via script tab.
  2. Load one built file via script tag
  3. Using an AMD loader, such as require.js, load one built file and add a shim property to require.js config so it knows the built file exports a sinon object.
  4. Using an AMD loader to require Sinon.JS file as individual modules.

Generating a single built file would be for items 2 and 3.

Would you consider changing the files to be Common.Js syntax? This would still allow generating a built file from browserify but AMD style folders/files could also be generated. Or the other way around and changing the files to be AMD syntax and generate Common.Js files. Either way should cover all four ways of loading.

This also means not having to split up the codebase into their own modules. One could just do

// common.js
var sinonSpy = require('Sinon.JS/spy')

// amd
require(['Sinon.JS/spy'], function(sinonSpy){...})

@cjohansen
Copy link
Contributor Author

CommonJS/Browserify is the goal.

@GCheung55
Copy link
Contributor

@cjohansen Cool. Just realized there is a sinon-2.0-more-strict branch and it already has the CommonJS style.

@vvo
Copy link

vvo commented Feb 12, 2015

@cjohansen I just released a standalone XHR/XDR mocking project.

It has some more precise features on XDR: currently sinon does not implement XDR "well", there should not be a readystate for example.

I also have .response, .timeout working for XHR.

It may not fit all your needs and lacks some features but it still extracted and so if you want to forkit/useit feel free: https://github.com/algolia/faux-jax

@mroderick
Copy link
Member

I just released a standalone XHR/XDR mocking project.

👍

@vvo
Copy link

vvo commented Feb 13, 2015

@mroderick @cjohansen finished "faux-jax", implement precisely from IE7 to 11.

IE does not requires to load additional files to overwrite XMLHttpRequest, may be useful for sinon, see this hack: https://github.com/algolia/faux-jax/blob/cc7243a35127cc6d944f72b1966fa4cac49f3f95/lib/ie.js

https://github.com/algolia/faux-jax/

@cjohansen
Copy link
Contributor Author

Interesting. If it solves all the cases using it in Sinon 2.0 will be a no-brainer. It saves us the work of extracting our own (incomplete) implementation.

@vvo
Copy link

vvo commented Feb 16, 2015

@cjohansen I am updating it frequentlty, it's all TDD, follow closely the specs (XHR, XDR). Currently missing:

Given the need, it may be trivial to implement it, did not have the use case.

@vvo
Copy link

vvo commented Feb 16, 2015

Also my hack can be used to overwrite setTimeouts etc.. without the need to load separate files. Also providing easy commonjs/browserify (where you only have one entry point) use

@mroderick
Copy link
Member

Given the need, it may be trivial to implement it, did not have the use case.

Perhaps some of the people asking for fully faked XHR2 api in Sinon.JS tickets could lend a hand if we campaign for it :)

@cjohansen
Copy link
Contributor Author

Also my hack can be used to overwrite setTimeouts etc.. without the need to load separate files. Also providing easy commonjs/browserify (where you only have one entry point) use

This was actually fixed in Sinon a while back as well, a release is pending.

@vvo
Copy link

vvo commented Feb 17, 2015

This was actually fixed in Sinon a while back as well, a release is pending.

Did you invent the trick or found it like me in dark places (google forums I guess)

@cjohansen
Copy link
Contributor Author

It was based on a very thorough article investigating the matter. I can't remember where I saw it, and unfortunately the commit does not honor the source :(

@GCheung55
Copy link
Contributor

If it's about overwriting setTimeout and such, I submitted a PR a few months ago... I know I included a link to the article.

Sent from my iPhone

On Feb 17, 2015, at 11:19 PM, Christian Johansen notifications@github.com wrote:

It was based on a very thorough article investigating the matter. I can't remember where I saw it, and unfortunately the commit does not honor the source :(


Reply to this email directly or view it on GitHub.

@GCheung55
Copy link
Contributor

It was #628 (comment)

@vvo
Copy link

vvo commented Feb 26, 2015

Made a module to allow overwriting writables: https://github.com/algolia/writable-window-method:

var writable = require('writable-window-method');

writable([
  'XMLHttpRequest',
  'Date',
  'setTimeout'
]);

function XMLHttpRequest() {
  console.log('Owned');
}

var noNative = new XMLHttpRequest();
// 'Owned'

var xhr = new writable.original.XMLHttpRequest();
// a real XMLHttpRequest instance

Does not requires loading additionnal files, it may be handy to sinon.js npm users because when using commonjs, you cannot load additional files easily.

@ponelat
Copy link

ponelat commented Jun 4, 2015

By supporting fakeServer on node.js would this allow for mocking nodej.js http.requests... using the same interface? Would mean that I can run http mock tests for nodejs and browser.... very cool.

@vvo
Copy link

vvo commented Jun 4, 2015

@ponelat This is exactly what you can do with https://github.com/algolia/faux-jax

@ponelat
Copy link

ponelat commented Jun 4, 2015

@vvo thanks, I'll take a look into that.

@mroderick
Copy link
Member

Can we close this PR now?

@cjohansen
Copy link
Contributor Author

Unfortunately, yes...

@cjohansen cjohansen closed this Sep 22, 2015
@vvo
Copy link

vvo commented Sep 22, 2015

I did not get it, why unfortunately? Sinon.js 2.0 will not make it as planned? (no big deal, only asking)

@cjohansen
Copy link
Contributor Author

Because I put some work into this, and now it is outdated. Attempting to reconcile it with current master will be a bigger job than redoing the work.

@cjohansen cjohansen deleted the sinon-2.0 branch November 10, 2015 23:15
@MattKunze
Copy link

Does this mean sinon 2.0 is being abandoned, or is there another issue we can use to track the progress towards that? I'm primarily asking regarding usage with webpack (webpack/webpack#117) - we've been pointing at a the 2.0 branch for our dependencies, but would prefer to keep it closer to the mainstream release if possible.

@cjohansen
Copy link
Contributor Author

The existing branch is abondoned, but there will be a Sinon 2.0 eventually. CommonJS modules is highly desired, but someone needs to put in some work for it to happen.

@frank-weindel
Copy link

I know Sinon 2.0 will happen eventually (and thank you for all the work you've put into this project!) but some people were relying on that branch for their webpack projects. For what I was using it for at least, it worked well.

I recovered the branch on a form, whose commits still exist on github, which anyone can use if they'd like, until the real Sinon 2.0 is released:
https://github.com/onetwosee/sinon/tree/sinon-2.0

@MattKunze
Copy link

FWIW var sinon = require('imports?define=>false,require=>false!sinon/pkg/sinon') is working for us with webpack and the current version of sinon (1.17.2), that's what we switched to when the 2.0 branch was deleted.

Pretty ugly, but I'd rather point to something closer to the mainstream development and then hopefully we can just clean up the require statement once CommonJS support exists

@frank-weindel
Copy link

Thanks for that! That's a much better way to go. You can also configure your webpack.config.js in such a way so that you can just require('sinon') in your test files. For anyone reading this, make sure you npm install imports-loader as a dev-dependency.

    module: {
      loaders: [
        {
            test: /sinon\.js$/,
            loader: 'imports?define=>false,require=>false'
        }
      ],
    },
    resolve: {
      alias: {
        sinon: 'sinon/pkg/sinon'
      }
    }

@vieron
Copy link

vieron commented Dec 7, 2015

Best solution for me (taken from webpack/webpack#304):

module: {
    noParse: [
        /node_modules\/sinon\//,
    ]
},
resolve: {
    alias: {
        'sinon': 'sinon/pkg/sinon'
    }
}

hpurmann added a commit to actano/yourchoice that referenced this pull request Jun 27, 2016
This plugin most likely doesn't work with karma >= 1.0.0
Source for this fix: sinonjs/sinon#600 (comment)
hpurmann pushed a commit to actano/yourchoice that referenced this pull request Jun 27, 2016
* chore(package): update karma to version 1.1.0

https://greenkeeper.io/

* Remove `karma-sinon-chai` dependency

This plugin most likely doesn't work with karma >= 1.0.0
Source for this fix: sinonjs/sinon#600 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants