calmm-js / partial.lenses

Partial lenses is a comprehensive, high-performance optics library for JavaScript
MIT License
915 stars 36 forks source link

Object.freeze #50

Closed polytypic closed 7 years ago

polytypic commented 7 years ago

Currently optics do not freeze the new objects and arrays they create, but it would not be difficult to make it so.

Personally I have not found it to be a problem that the resulting objects are mutable (I just don't mutate them), but I can see that in some contexts it could be helpful to have the result objects frozen.

A problem with Object.freeze is that it can be expensive in some JS engines. So, I would not use it in production builds unless there was a really good reason to do so.

So, one approach might be to have an additional opt-in configuration environment variable that can be explicitly defined, like PARTIAL_LENSES__FREEZE=1, which results in a build of the library that freezes any new objects returned by optics. However, by default, even in non-production builds, new objects would not be frozen.

polytypic commented 7 years ago

Contrary to what I previously wrote, I've been thinking about making it so that the next major version would freeze any newly constructed objects and arrays returned by optics in non-production builds. Why only non-production builds? Because I want to avoid creating many builds of this library and because freezing can be quite expensive.

The benefit of this would be to strongly guide one away from mutating data structures and to help catch bugs. The disadvantage is that there might be legitimate cases where one does want/need to be able to mutate things.

Opinions and feedback on this issue are welcome! If you just want to express approval (you want to guide away from mutation) set 👍 and 👎 for disapproval (you want to remain open to potential mutation).

skokenes commented 5 years ago

It would be nice if there were a way to disable this feature. I've hit a use-case where I need the data mutated:

I am using vega.js to do data visualization. I need to transform a complex data set into a more usable format by vega, and then pass that data to vega. When vega gets the data, it mutates it for its own purposes. I don't have any way to control that as far as I know. Using partial.lenses to transform my data has made my life way easier in getting the data into the correct format, but now I'm forced in development at least to deep clone the data before I pass it to vega, which I imagine is more expensive than if I could just pass the data directly?

geoffreytools commented 3 years ago

I ran into a similar issue when writing a Babel plugin: I use partial.lenses to build the new node, but then Babel must be mutating it because I get the dreaded "Cannot assign to read only property '2' of object '[object Array]'" error unless I do a shallow copy of L.modify's return value .

It is not that big of a deal performance-wise, but having to do it does feel wrong, and the fact that Babel plugins can be used in a development environment as well shows the limit of coupling this configuration setting with NODE_ENV.

I understand that enabling structural sharing depends on data being immutable so I'm not suggesting that Object.freeze is to be removed, but maybe your first instinct of using a specific explicit environment variable was a better idea.