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 194 forks source link

RegExps are broken when made immutable #135

Closed holyjak closed 8 years ago

holyjak commented 8 years ago

I expect these two pieces of code to yield the same result:

/ABC/.test("ABC")
// => true

Immutable({regexp: /ABC/}).regexp.test("ABC")
// => TypeError:  Immutable(...).regexp.test is not a function

but they do not.

Ideally, seamless-immutable should not break regular expressions. If that is not possible because they are inherently mutable (as I suspect) that it should perhaps not just silently break them.

Possible courses of action (not complete, not exclusive):

  1. Make RegExp work! (likely impossible because they are mutable by nature)
  2. Warn when trying to Immutable-ize something that would be broken by it (such as RegExp or e.g. Node https.Agent)
  3. Or deny to do so and just throw an error (that is likely too aggressive)
  4. Enable the user to provide a custom "converters" to handle these special cases, e.g. something like: Immutable(objWithRegExps, { "RegExp": (v) => v /* return unmodified, i.e. mutable*/}) and then, somewhere in the code, if (customConverters[current.constructor.name]) return customConverters[current.constructor.name](current);

Thoughts / advice?

LinusU commented 8 years ago

RegExp is immutable by default as far as I understand it.

var a = new RegExp('a')

a.flags
// ""
a.flags = 'g'
// "g"
a.flags
// "" (still empty)

a.source
// "a"
a.source = 'b'
// "b"
a.source
// "a" (not modified)
corymartin commented 8 years ago

This issue is also present with Errors - which is how I found my way here. Perhaps a solution like that in #31?

rtfeldman commented 8 years ago

I just looked into this in more depth and I realized it should be closed as a duplicate of https://github.com/rtfeldman/seamless-immutable/issues/124

holyjak commented 8 years ago

@rtfeldman So this is duplicate of #124 but that was closed with "won't fix" with the explanation that Immutable should only store plain old data - i.e. that regular expressions, Errors etc. can not be included in an Immutable data structure. Correct?

Perhaps, if that is the case, it should be clearly documented in the README and, even better, an exception should be thrown when trying to include any other then supported type of data (i.e. fail quickly and with a clear error message then later with a mysterious error). What do you think?

Also, according to the README, it is possible to include "mutable" elements of type Date, function, React component. That provides a simple workaround: instead of using prop: RegExp(..), I could use () => new RegExp(). But wouldn't it be even better to expand this and allow the user to define what "mutable" data can be part of an Immutable data structure? Something like I proposed in 4. above or something similar (a list of types/paths to leave as they are ...).

Thoughts?