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

Clarity about using optics with arrays #73

Closed
joshburgess opened this issue Dec 29, 2018 · 3 comments
Closed

Clarity about using optics with arrays #73

joshburgess opened this issue Dec 29, 2018 · 3 comments
Assignees

Comments

@joshburgess
Copy link

joshburgess commented Dec 29, 2018

Okay, first: I found this PR #48 and discovered that I can use arrayIndex to set/modify values at particular indices in an array, like:

const arr = ['a', 'b', 'c']

indexArray<string>().index(0).set('x')(arr)
// => ['x', 'b', 'c']

...but I'm a little confused, because Lenses can also operate on arrays. However, they aren't quite right, because they convert them into object representations!

fromProp<string[]>()(0).set('x')(arr)
// => { 0: 'x', 1: 'b', 2: 'c' }

However, using a Traversal works as expected and returns an actual array:

fromTraversable(array)<string>().set('x')(arr)
// => ['x', 'x', 'x']

Based on the above, I would expect to be able to use lenses to set/modify values at particular indices in an array & receive back an actual array.

Is this expected or a mistake? Would it be possible to detect when a Setter is acting on an Array and perform an extra step that converts the object representation back into an actual array? Something like Object.values(result) right before returning? Keep in mind, the types already show that you are receiving back an actual array, but if you go to log the result out at runtime you can see that it's actually been converted into an object.

This led me to believe that this behavior is a bug, but, then, I discovered the Index type & arrayIndex, which provide a completely different way to accomplish the same thing without the same quirk.

So, I just wanted to create this issue to clarify. Thoughts?

@gcanti
Copy link
Owner

gcanti commented Dec 29, 2018

Accessing elements of an array is not modeled by a Lens (it's an Optional), so fromProp shouldn't be used

// lens: Lens<string[], string> <= this is not correct
const lens = Lens.fromProp<string[]>()(0)

// astring: string
const astring = lens.get([])

console.log(astring) // => undefined

@joshburgess
Copy link
Author

joshburgess commented Dec 29, 2018

@gcanti Ok, that makes sense. I understand that you have to account for the array being empty, the index being missing or out of bounds, etc.

So, this works as expected:

const headOptional = new Optional<string[], string>(head, a => xs =>
  tail(xs).fold(array.of(a), ys => cons(a, ys)),
)

headOptional.set('x')(arr)
// => ['x', 'b', 'c']

However, there is also Optional.fromNullableProp:

Optional.fromNullableProp<string[]>()(0).set('x')(
  arr,
)
// => { 0: 'x', 1: 'b', 2: 'c' }

This has the same problem as Lens.fromProp. It seems to work due to the definition using keyof & arrays being implemented as objects under the hood in JS, but, again, returns the object representation instead of an actual array.

I understand that Lens.fromProp, Optional.fromNullableProp, etc. are probably only intended to be used with actual objects, but do you think that, maybe, the library should prevent the user from using them with arrays instead of doing what it does now?

Perhaps, these sorts of functions could use conditional types to detect whether or not the type you passed in is an array? Something like...

S extends unknown[] ? never : /* original return type here */

I think this would be nice, because it would disallow improper use & guide the user towards (hopefully) finding the right functions/optics to use.

@gcanti
Copy link
Owner

gcanti commented Dec 29, 2018

but do you think that, maybe, the library should prevent the user from using them with arrays instead of doing what it does now?

Absolutely, I don't know how to do that though, there are possibly many other unsuitable data structures.

However, until we'll figure out something better, we could try to disallow the most common misuses, namely

  • arrays
  • tuples
  • Record<string, V>

gcanti added a commit that referenced this issue Dec 29, 2018
- Lens.fromProp
- Lens.fromProps
- Lens.fromPath
- Lens.fromNullableProp
- Optional.fromNullableProp
- Optional.fromOptionProp
@gcanti gcanti self-assigned this Dec 29, 2018
@gcanti gcanti closed this as completed in dd5fff7 Jan 2, 2019
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

2 participants