Use Iterable.isIterable for Immutable check #36

jakelazaroff · 2015-12-12T21:05:42Z

Right now the check for whether an object is Immutable uses instanceof Collection, which returns false if different modules are using different copies of Immutable.

This PR replaces that with Immutable.Iterable.isIterable as advised here: http://stackoverflow.com/questions/31907470/how-to-check-if-object-is-immutable

astorije · 2015-12-12T23:07:42Z

Hey @jakelazaroff, thanks for your contribution! Good catch too!
Before I can merge this, could you think of a test case where the current solution fails and yours passes please? :-)

jakelazaroff · 2015-12-13T01:41:53Z

No problem 😁 I'll update the PR in a few minutes.

jakelazaroff · 2015-12-13T02:08:44Z

test/test.js

@@ -68,6 +71,12 @@ describe('chai-immutable (' + typeEnv + ')', function () {
      it('should fail using `not` given an empty collection', function () {
        fail(function () { expect(new List()).to.not.be.empty; });
      });
+
+      if (typeEnv === 'Node.js') {


I couldn't figure out how to load a different copy of Immutable for the Phantom tests, but I don't think it should really be a problem anyway since in a browser everything will just use whatever's on the window object. I also don't love wrapping its inside a conditional — let me know if there's a way you'd rather I write this!

jakelazaroff · 2015-12-13T02:18:00Z

@astorije Updated the PR but there's some work to be done still. After adding the tests I realized there were more instanceofs to replace, which for the most part was straightforward except for the keys method: https://github.com/astorije/chai-immutable/blob/master/chai-immutable.js#L220

It's not exactly clear (to me, at least) how these map to the static methods listed here: https://facebook.github.io/immutable-js/docs/#/Iterable/isIterable I'm guessing instanceof KeyedCollection could be replaced with isKeyed and instanceof IndexedCollection could be replaced with isIndexed, but I'm not sure about instanceof SetCollection (isAssociative, maybe?)

jakelazaroff · 2015-12-13T02:25:31Z

Also: trying to figure out now how to replace these assertions with the Iterable static methods: https://github.com/astorije/chai-immutable/blob/master/chai-immutable.js#L312

jakelazaroff · 2015-12-15T06:31:12Z

@astorije Should be fixed! I replaced the new Assertion checks with this: https://github.com/astorije/chai-immutable/pull/36/files#diff-27f88d01c83c19b108b361c0975ed547R27

For the keys method, I compared the instanceof checks with this table:

Looks like anything that returns true for isIndexed you can call toJS on and it'll get the keys, so that check now catches List and Stack. The rest just fall through to a general isIterable check before they get to Object.keys.

Let me know if there's anything you'd like me to add/change 😄

jakelazaroff · 2016-01-06T03:58:40Z

Anything I can do to help get this merged?

astorije · 2016-01-06T04:33:02Z

Hi @jakelazaroff, so sorry I have been very busy and I must admit I dropped the ball, shame on me...
I can't take a close look at that right now, but I'll try within a week or two. If I don't, well, I will have dropped the ball again... you know where to ping me!

I don't like the idea of wrapping it in conditions. I'll think of something but at the very least this.skip(); looks better to me (I was not too happy with typeEnv when I added it but I found it a cheap way to display the env). Of course, none of these are ideal, it would be better to actually make the tests relevant on all environments. Again, I'll think of something.
For the rest, well, I need to spend time on this :-)

Thanks again!

jakelazaroff · 2016-01-06T15:35:48Z

No worries! I'll try to figure out a way to get Phantom to load two different copies of Immutable; that should avoid the test-skipping issue entirely.

astorije · 2016-01-17T18:59:37Z

test/test.js

@@ -5,6 +5,7 @@ if (!chai) {
  var chai = require('chai');
  var chaiImmutable = require('../chai-immutable');
  var Immutable = require('immutable');
+  var otherImmutable = require('../node_modules/immutable/dist/immutable.min.js');

  chai.use(chaiImmutable);
  typeEnv = 'Node.js';


Another way to do that, somewhat cleaner and most likely compatible with PhantomJS, but probably a bit more difficult to achieve, is to deep clone/duplicate Immutable right before var assert = chai.assert; and assign this copy to your otherImmutable (that could be renamed clonedImmutable accordingly).

Then I believe you can remove all those if (typeEnv === 'Node.js') and let the runners test these.

Would you mind exploring this idea before we can merge?

Thanks!

astorije · 2016-01-17T19:01:59Z

@jakelazaroff, I left you a comment inline that might address this problem. Let me know what you think and when you think you'll reach to a solution: I'm sorry to see your PR hanging around and would like to help you having it ready for production!

astorije · 2016-01-28T05:37:48Z

@jakelazaroff, do you have any update on this? :-)

astorije · 2016-02-15T00:01:20Z

@jakelazaroff, ping ping ping? :-)

jakelazaroff · 2016-02-17T04:47:55Z

@astorije Crap, I seriously dropped the ball on this. I'll update by the end of the week. Sorry about the delay!

jakelazaroff · 2016-02-22T03:36:18Z

@astorije Your hunch worked! Should be good to go now.

astorije · 2016-03-12T22:04:42Z

test/test.js

@@ -68,6 +82,10 @@ describe('chai-immutable (' + typeEnv + ')', function () {
      it('should fail using `not` given an empty collection', function () {
        fail(function () { expect(new List()).to.not.be.empty; });
      });
+
+      it('should work if using different copies of Immtuable', function () {


Typo at Immtuable.

astorije · 2016-03-12T22:12:36Z

Hi @jakelazaroff, sorry I didn't get back to you earlier!
It starts to look good, almost there! I left you a few comments inline, basically about typos and the fact that tests probably don't reflect the exact issue you opened your PR with.

Also, could you squash your commits? :-)
I'll try to be more responsive next time!

jakelazaroff · 2016-03-13T23:28:51Z

Done! I think this should do it :)

astorije · 2016-03-14T00:04:16Z

Very nice job @jakelazaroff, sorry it took so long!
👍

Use Iterable.isIterable for Immutable check

jakelazaroff · 2016-03-15T03:59:21Z

Awesome, thanks so much!

Side note: if you ever need to use this with karma, I created this adapter for it https://github.com/jakelazaroff/karma-chai-immutable

jakelazaroff mentioned this pull request Dec 12, 2015

Doesn't work if multiple copies of Immutable are used jakelazaroff/karma-chai-immutable#1

Closed

jakelazaroff force-pushed the use-is-iterable branch from b34220f to ef282de Compare December 13, 2015 02:05

jakelazaroff reviewed Dec 13, 2015
View reviewed changes

jakelazaroff force-pushed the use-is-iterable branch from ef282de to 047f61b Compare December 13, 2015 02:09

jakelazaroff force-pushed the use-is-iterable branch from 38d485d to feeb3af Compare December 15, 2015 06:21

astorije reviewed Jan 17, 2016
View reviewed changes

astorije reviewed Mar 12, 2016
View reviewed changes

Use Iterable.isIterable for Immutable check

7c8697f

jakelazaroff force-pushed the use-is-iterable branch from d8e2005 to 7c8697f Compare March 13, 2016 08:29

astorije added a commit that referenced this pull request Mar 14, 2016

Merge pull request #36 from jakelazaroff/use-is-iterable

f77dbc7

Use Iterable.isIterable for Immutable check

astorije merged commit f77dbc7 into astorije:master Mar 14, 2016

jakelazaroff deleted the use-is-iterable branch March 15, 2016 03:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Iterable.isIterable for Immutable check #36

Use Iterable.isIterable for Immutable check #36

jakelazaroff commented Dec 12, 2015

astorije commented Dec 12, 2015

jakelazaroff commented Dec 13, 2015

jakelazaroff Dec 13, 2015

jakelazaroff commented Dec 13, 2015

jakelazaroff commented Dec 13, 2015

jakelazaroff commented Dec 15, 2015

jakelazaroff commented Jan 6, 2016

astorije commented Jan 6, 2016

jakelazaroff commented Jan 6, 2016

astorije Jan 17, 2016

astorije commented Jan 17, 2016

astorije commented Jan 28, 2016

astorije commented Feb 15, 2016

jakelazaroff commented Feb 17, 2016

jakelazaroff commented Feb 22, 2016

astorije Mar 12, 2016

astorije commented Mar 12, 2016

jakelazaroff commented Mar 13, 2016

astorije commented Mar 14, 2016

jakelazaroff commented Mar 15, 2016

Use Iterable.isIterable for Immutable check #36

Use Iterable.isIterable for Immutable check #36

Conversation

jakelazaroff commented Dec 12, 2015

astorije commented Dec 12, 2015

jakelazaroff commented Dec 13, 2015

jakelazaroff Dec 13, 2015

Choose a reason for hiding this comment

jakelazaroff commented Dec 13, 2015

jakelazaroff commented Dec 13, 2015

jakelazaroff commented Dec 15, 2015

jakelazaroff commented Jan 6, 2016

astorije commented Jan 6, 2016

jakelazaroff commented Jan 6, 2016

astorije Jan 17, 2016

Choose a reason for hiding this comment

astorije commented Jan 17, 2016

astorije commented Jan 28, 2016

astorije commented Feb 15, 2016

jakelazaroff commented Feb 17, 2016

jakelazaroff commented Feb 22, 2016

astorije Mar 12, 2016

Choose a reason for hiding this comment

astorije commented Mar 12, 2016

jakelazaroff commented Mar 13, 2016

astorije commented Mar 14, 2016

jakelazaroff commented Mar 15, 2016