Support for static syntax without object instance methods

[ghost1542]

This PR allows static functions to continue working when overriding reserved keys such as setIn with custom data in the objects.

Previously custom data would be dropped. Now custom data would override instance functions. Neither are good, which is why this PR is including all steps to migrate existing users to the new syntax.

Backward compatibility is maintained, but users using the old syntax would get this kind of warning DEPRECATED: Using x.asObject(...) will be deprecated. Please use Immutable.asObject(x, ...) instead.

[ghost1542]

@suchipi Thanks, just applied all requested changes and added more test units.

[ghost1542]

@rtfeldman Note; The migration to the new syntax is optional. I can easily drop the console.warn warnings from this PR if you prefer to keep supporting both syntax going forward.

[rtfeldman]

After a lot of thought, I don't think outright migrating to this syntax is the right move. I think it would be better to make this opt-in, for example:

var Immutable = require("seamless-immutable").static;

Making this give you a new version of Immutable that doesn't override methods seems like the best way to go. It preserves backwards compatibility and would be easy to opt into.

What do you think?

[ghost1542]

@rtfeldman I have dropped the deprecation warning from this pull request such that it is now mostly only #130 bugfix and test units for it, and not a migration scenario.

This PR still includes this breaking change. However in my views, losing data or losing access to the method are both undesirable bugs, and this change is the key to allow the user to switch to a static syntax that doesn't have the bug if needs be. Thus in my humble opinion merging this would be an improvement.

I gave a good hour at exploring the idea of require("seamless-immutable").static; to address #170. This seems like a good idea at first, however this appeared non-trivial to me, as Immutable(), makeImmutableArray(), makeImmutableObject() and such are called generously from within the library. It appears pretty easy to miss a case and introduce bugs, or re-pollute the object. And maybe this may turn into a much bigger refactoring.

Although I do understand that it is great to support previous syntax, I am generally a fan of keeping things simple to ease maintainability and avoid bugs. It seems like there is no argument in favor of the previous syntax beside that it's currently in use (which also suggests it should not be promoted going forward), the static syntax appears superior on all fronts. So I tend to like the idea of migrating :) . But it's just me.

[ghost1542]

tl;dr; I suggest we only merge bugfixes, and choose between migrating or not migrating on a separate PR.

Edit: Docs were updated to refer to the static syntax in this PR, I can revert that change if asked for it. Although since it's already released and more stable, sounds like a better choice to me.

[ghost1542]

@rtfeldman I may have found a simple compromise that is similar to what you are suggesting. I pushed another commit for this

https://github.com/saivann/seamless-immutable/tree/collision#static-or-instance-syntax

var Immutable = require("seamless-immutable");
Immutable.config({
    use_instance_methods: false
});

[ghost]

@saivann would the config be opt-in or opt-out?

[ghost1542]

@MoeSattler, the config only defines if seamless-immutable should add sub-methods to all objects it creates. This is opt-in (no change to current Immutable default behavior). In summary, in this PR:

1. Seamless-immutable continues to support the same syntaxes it already supports

   Immutable.setIn(obj, 'key', data)

   obj.setIn('key', data)

2. In case of collision, user data now has priority over seamless-immutable methods:

Previously:

var obj = Immutable({setIn: 'data'})
obj.setIn; // function, the assigned data is lost

Now:

var obj = Immutable({setIn: 'data'})
obj.setIn; // 'data', the seamless-immutable method is lost, user can use static functions

3. User can opt-in to stop attaching seamless-immutable methods to objects

var Immutable = require("seamless-immutable");
Immutable.config({
    use_instance_methods: false
});

var obj = Immutable({})
obj.setIn; // undefined, the object is clean

[suchipi]

To remain completely backwards compatible ("bugwards compatible"), the second item in your summary would need to be reverted to the old behavior; for users who need a workaround for data loss, I think the static methods should be recommended instead (or a config key could be added to prioritize user data instead of methods).

Even though prioritizing user data over instance methods is a nicer API in an ideal world... maintaining backwards compatibility and avoiding a major version bump are important from a package maintainer's perspective, too.

[ghost1542]

@suchipi Although I can easily make this conditional to the use_instance_methods config if I am asked for it, I think this is actually a bad idea that I'd like to discourage, because it makes it harder for new users to have bug-free library by default. Users using the static syntax would still be affected unless they also set use_instance_methods, which I think we can safely assume most users won't.

And perhaps obvious, but it seems unnecessary to me that we keep backward compatibility with a bug. There is no perfect bug-free backward compatible solution for the existing syntax. But we can however make sure the library is bug-free for new users, while supporting existing syntax and having a solution at hands for any existing users who may be affected this bug. This is what I am recommending in this PR.

[ghost1542]

Actually, I am changing my mind on this ^ . There are security implications with allowing users to run an instance method, while at the same time allowing arbitrary override of those methods. I'll update the PR and try to come up with something better.

[ghost1542]

OK latest attempt:

1. Seamless-immutable continues to support the same syntaxes it already supports

   Immutable.setIn(obj, 'key', data)

   obj.setIn('key', data)

2. In case of collision, user data is lost, no matter what syntax (no change with current spec):

var obj = Immutable({setIn: 'data'})
obj.setIn; // function, the assigned data is lost

3. User can opt-in to stop attaching seamless-immutable methods to objects

var Immutable = require("seamless-immutable").init({
    use_static: true
});

var obj = Immutable({})
obj.setIn; // undefined, the object is clean

This is pretty in line to what @rtfeldman asked (sorry for all the extra loops before getting there!).

I still think it'd be great to migrate to one syntax in the future, but can be for later.

[rtfeldman]

@saivann This is great! 😍 Thank you so much!

My only request would be to simplify it to just:

var Immutable = require("seamless-immutable").static;

I don't anticipate adding more configuration options later, so having an init function feels like overkill.

Sound good?

[ghost1542]

@rtfeldman Just added a commit for this, thanks! Looking forward to use seamless-immutable 6.4.0 :)

[ghost]

Love this! Thanks so much!

[rtfeldman]

@saivann Thanks! I'm going to pair this with #177 and release as 7.0.0

[rtfeldman]

Released as 7.0.0.

[draunkin]

Thanks so much for this. I was developing some reporting in our application which uses data that is not frequently accessed and only then did I realise this was happening to some of my objects which have since been serialized into the db. ... well can't reverse time but so glad you've already got a solution for it!