Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optic doesn't support Symbol. #166

Open
placidusax-in-storm opened this issue Apr 8, 2018 · 8 comments
Open

Optic doesn't support Symbol. #166

placidusax-in-storm opened this issue Apr 8, 2018 · 8 comments

Comments

@placidusax-in-storm
Copy link

Well, Optic is support String, Array ...etc
However, the ES6 new type Symbol is mostly support by modern browser and Node.js.
But, if we create a new Optic like this:

[
  Symbol(),
  'string'
]

The error is throw.

@polytypic
Copy link
Member

polytypic commented Apr 8, 2018

Yes, currently Partial Lenses do not support symbols and in non-production mode the library raises an exception as seen here.

I'm not opposed to supporting symbols, but I'm not entirely sure what the best way to support them would be. Ramda, for example, does not currently support symbols. It seems that the use case of symbols is to avoid name clashes. Even JavaScript itself seems rather undecided on how symbol properties should be treated: object spread ignores them, while Object.assign copies them as seen here. Object spread and Object.assign copy symbols as seen here.

I did some quick tests and it seems that copying symbol properties (using builtin Object.assign or Object.getOwnPropertySymbols) would cause something like a 2x performance hit in latest Node for property lenses. Note that I mean this in the sense of supporting the possibility of having symbol properties—I actually tested this on objects that did not have symbol properties, so there was no extra cost of actually copying more. Hopefully future JS engines will have faster implementations of Object.assign and then supporting symbol properties could be done without taking a performance hit.

For the above two reasons, namely

  • that there currently does not seem to be a consensus on what the best way to support them is (copy or not copy), and
  • that copying them (it seems) could could cause a 2x performance hit,

I'd rather not enhance the default property lenses in Partial Lenses to support symbol properties at this point. The above concerns may change in the future (IOW, a consensus may be reached and copying symbol properties might get significantly faster) at which point this could be reconsidered.

Note that you can always write custom optics for accessing objects with symbol properties. Here is a simple example using L.lens and here is a version that also provides the symbol or string as the index (Oops: Fixed!). At the moment this is what I'd recommend if you need to access objects with symbol properties.

Also, it might be useful to add the above sym lens to the library. Possibly with a better name? Name suggestions and/or PR is welcome!

@placidusax-in-storm
Copy link
Author

I read your example. However, the variable name like xi2yF confuse me. What it's name mean? When I want to read this project source code. This problem tangle me, I want to contribute my code, but first , how to understand this, which knowledge I miss. Thanks

@polytypic
Copy link
Member

Ah... Good question. xi2yF spells out the signature of the parameter: it is a function that maps the value under scrutiny x and its index i to the resulting value y in functor F.

@polytypic
Copy link
Member

Also, my latter example had a bug—I just fixed the link.

@placidusax-in-storm
Copy link
Author

Got it ,thanks

@Justin-ZS
Copy link

Justin-ZS commented May 7, 2018

@polytypic
Hi, I think the object spread demo should be {...x, y: 1} rather than {...foo, y: 1}.
In this case, the behavior is same to Object.assign

@polytypic
Copy link
Member

@Justin-ZS Oops. Thanks for the correction! Here is the fixed playground.

@ftaiolivista
Copy link

If someone, like me, needs to not loose Symbols using lenses this a patch I have done to infestines.


diff --git a/node_modules/infestines/dist/infestines.cjs.js b/node_modules/infestines/dist/infestines.cjs.js
index 3d711b4..a1a4f30 100644
--- a/node_modules/infestines/dist/infestines.cjs.js
+++ b/node_modules/infestines/dist/infestines.cjs.js
@@ -438,14 +438,8 @@ var assocPartialU = function assocPartial(k, v, o) {
   var r = {};
   if (o instanceof Object) {
     if (!isObject(o)) o = toObject(o);
-    for (var l in o) {
-      if (l !== k) {
-        r[l] = o[l];
-      } else {
-        r[k] = v;
-        k = void 0;
-      }
-    }
+    // Symbol Support
+    Object.assign(r, o);
   }
   if (isDefined(k)) r[k] = v;
   return r;
@@ -456,10 +450,9 @@ var dissocPartialU = function dissocPartial(k, o) {
   if (o instanceof Object) {
     if (!isObject(o)) o = toObject(o);
     for (var l in o) {
-      if (l !== k) {
-        if (!r) r = {};
-        r[l] = o[l];
-      } else {
+      // Symbol Support
+      if (!r) {r = {...o}}
+      if (l === k) {
         k = void 0;
       }
     }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants