rtfeldman / seamless-immutable

Immutable data structures for JavaScript which are backwards-compatible with normal JS Arrays and Objects.
BSD 3-Clause "New" or "Revised" License
5.37k stars 195 forks source link

Immutable.from (a reference to Immutable) #126

Closed ericclemmons closed 8 years ago

ericclemmons commented 8 years ago

This is a PR in response to #123, which allows users who follow & lint the condition that new precedes any capitalized, constructor function to use this library without littering their codebase with // eslint-disable-line comments:

http://eslint.org/docs/rules/new-cap

Before:

const thing = Immutable({}); // eslint-disable-line

After:

const thing = Immutable.from({});

I did a find/replace for = Immutable( to = Immutable.from in test/, and all 2108 tests still pass.

ericclemmons commented 8 years ago

However, new Immutable(...) would also satisfy this rule, but would fail 4 tests:

  2104 passing (8s)
  4 failing

What would you prefer? Immutable.from or supporting instantiation via new Immutable?

rtfeldman commented 8 years ago

new Immutable seems better, assuming it can be done in a way that doesn't break stuff (not sure which tests are failing, and whether it's the test's fault or the new logic's fault 😉 )

ericclemmons commented 8 years ago

@rtfeldman I agree. I'll post up more info. I'm a fan of new Immutable personally, but I know some people are completely anti-instantiation. But those people aren't the ones with the new-cap rule ;)

ericclemmons commented 8 years ago

Replacing the tests to use new Immutable yields:

  2104 passing (9s)
  4 failing

  1) Development build ImmutableArray returns only immutables when you call its reduce() method:

      AssertionError: expected '{}' to equal '0.10615249630063772'
      + expected - actual

      -{}
      +0.10615249630063772

      at Function.assert.strictEqual (node_modules/chai/lib/chai/interface/assert.js:178:32)
      at assertJsonEqual (test/TestUtils.js:13:12)
      at Object.assertImmutable (test/TestUtils.js:22:5)
      at Function.<anonymous> (test/ImmutableArray/test-compat.js:179:19)
      at Function.<anonymous> (test/TestUtils.js:121:18)
      at check (test/TestUtils.js:103:13)
      at test/TestUtils.js:117:7
      at Context.<anonymous> (test/ImmutableArray/test-compat.js:177:7)

  2) Development build ImmutableArray returns only immutables when you call its reduceRight() method:

      AssertionError: expected '{}' to equal '0.8079638944473118'
      + expected - actual

      -{}
      +0.8079638944473118

      at Function.assert.strictEqual (node_modules/chai/lib/chai/interface/assert.js:178:32)
      at assertJsonEqual (test/TestUtils.js:13:12)
      at Object.assertImmutable (test/TestUtils.js:22:5)
      at Function.<anonymous> (test/ImmutableArray/test-compat.js:179:19)
      at Function.<anonymous> (test/TestUtils.js:121:18)
      at check (test/TestUtils.js:103:13)
      at test/TestUtils.js:117:7
      at Context.<anonymous> (test/ImmutableArray/test-compat.js:177:7)

  3) Production build ImmutableArray returns only immutables when you call its reduce() method:

      AssertionError: expected '{}' to equal 'false'
      + expected - actual

      -{}
      +false

      at Function.assert.strictEqual (node_modules/chai/lib/chai/interface/assert.js:178:32)
      at assertJsonEqual (test/TestUtils.js:13:12)
      at Object.assertImmutable (test/TestUtils.js:22:5)
      at Function.<anonymous> (test/ImmutableArray/test-compat.js:179:19)
      at Function.<anonymous> (test/TestUtils.js:121:18)
      at check (test/TestUtils.js:103:13)
      at test/TestUtils.js:117:7
      at Context.<anonymous> (test/ImmutableArray/test-compat.js:177:7)

  4) Production build ImmutableArray returns only immutables when you call its reduceRight() method:

      AssertionError: expected '{}' to equal '""'
      + expected - actual

      -{}
      +""

      at Function.assert.strictEqual (node_modules/chai/lib/chai/interface/assert.js:178:32)
      at assertJsonEqual (test/TestUtils.js:13:12)
      at Object.assertImmutable (test/TestUtils.js:22:5)
      at Function.<anonymous> (test/ImmutableArray/test-compat.js:179:19)
      at Function.<anonymous> (test/TestUtils.js:121:18)
      at check (test/TestUtils.js:103:13)
      at test/TestUtils.js:117:7
      at Context.<anonymous> (test/ImmutableArray/test-compat.js:177:7)

From what I can tell it's the nonMutatingArrayMethods

rtfeldman commented 8 years ago

Oh right...if you use new, you have to get an object back; it can't return plain strings, numbers, null, etc. - all of which are currently supported (and passed through unchanged).

Let's just go with "from" after all haha

ericclemmons commented 8 years ago

Oh right! Travis is failing do to the Zuul testing (likely because my repo doesn't have the env vars yours does for testing).

Let me know what else you'd like @rtfeldman :)

rtfeldman commented 8 years ago

Looks good! Last request would be a quick mention in the README. 😸

ericclemmons commented 8 years ago

@rtfeldman Let me know if you'd like any other changes!

https://github.com/ericclemmons/seamless-immutable/blob/123-from/README.md#immutablefrom

rtfeldman commented 8 years ago

Lovely, thank you! :smile:

rtfeldman commented 8 years ago

@ericclemmons published as 6.1.0!