Skip to content
This repository was archived by the owner on Apr 15, 2025. It is now read-only.

Should with even be a part of this proposal? #41

Closed
TehShrike opened this issue Aug 16, 2019 · 19 comments
Closed

Should with even be a part of this proposal? #41

TehShrike opened this issue Aug 16, 2019 · 19 comments

Comments

@TehShrike
Copy link
Contributor

TehShrike commented Aug 16, 2019

Continuing a thread from #1 – is there a compelling reason to include with in this proposal?

I don't believe this proposal needs the with syntax to be viable. I also doubt that the proposed with syntax needs to be specific to immutable data structures – someObject with .b=5 would be handy as sugar for Object.assign({}, someObject, { b: 5 }).

Context:

@ceigey
Copy link

ceigey commented Aug 17, 2019

I feel like having the two together is a good idea; otherwise, you might end up with a situation where, despite immutable data structures existing in the runtime, people are using good ol fashioned pretend-immutable data structures with libraries like Immer for the better developer ergonomics when making deep changes instead.

But, I do also feel like having something akin to Immer built in to JS is a great idea on its own too, and this repurposing of the with keyword provides that.

Speaking of deep property access, both the core proposal and the with syntax make me wonder how they'd interact with other proposals, e.g. what happens to ?. and when the path the mutation is being made at doesn't exist; could ?. be used to automatically fill in said path? And if so, do we assume that tuples are secretly a subset of records and records can be made from tuples?

So... if the proposal was split up, I wonder how deep their shared concerns would be; but evidently there's already other proposals that are completely separate that would have to be considered too.


Though, if the use of with as a keyword was replaced with some sort of function or method that had a similar set of ergonomics, that might make it easier to keep things all in the same proposal as there'd be less of an uphill battle.

@TehShrike
Copy link
Contributor Author

TehShrike commented Aug 17, 2019

zenflow brought up that the spread syntax already exists and would accomplish the same thing without adding a new language feature.

@ceigey
Copy link

ceigey commented Aug 17, 2019

Spread syntax is ergonomic at one level deep, but after that, while perfectly functional, the syntax is not ideal and requires a fair bit of repetition.

Considering:

let myRec = #{
  user: {
    address: {
      street: 'First St',
      //...
    }
    //...
  }
  //...
}

let newRecord = #{
  ...myRec,
  user: {
    ...myRec.user,
    address: {
      ...myRec.user.address,
      street: 'Second Ave'
    }
  }
}

The update would be better as something like:

myRec.user.address.street = 'Second Ave';

... but that won't fly because we're dealing with immutable objects and the assignment operator can't "return" a value to assign to something else.

Without a better way to update deeply nested properties in a record, I'd rather stick to immer and mutable objects for those use cases, and/or avoid nested records and tuples entirely.

(Perhaps nested records would be considered bad practice by some anyway, but that's yet another debate)

Edit: forgot where I was going with this - I guess the question I'm pondering is this:
Should this proposal reach Stage 2 or 3 without any way to do convenient deep property updates on the horizon, would that be a significant missed opportunity?

@ljharb
Copy link
Member

ljharb commented Aug 17, 2019

then rather than with, I’d prefer to see + used for that use case (and ideally - as well, if possible)

@ceigey
Copy link

ceigey commented Aug 17, 2019

As in

let newRec = #{ x: { val: 2 }, y: 5 } + #{ x: { val: 0 } };
newRec === #{ x: { val: 0 }, y: 5 };

... or ....

let newRec = #{ x: { val: 2 } } + .x.val = 0

(Or something else?)
?

(I'm assuming let newRec = rec.x.val + 0 would be too ambiguous and confusing)

@ljharb
Copy link
Member

ljharb commented Aug 17, 2019

The former, and yes, ideally the operator would throw if you tried to combine an immutable type with a non-immutable one.

@dead-claudia
Copy link
Contributor

Related: #19

@ceigey
Copy link

ceigey commented Aug 18, 2019

@ljharb hmm interesting, I like it. It reinforces that there's certain special properties to these data structures too, and gets rid of the need for additional syntax for deep updates.

Have you made a separate issue for the use of +/- already? Should we break discussion about that into a new issue?

Main concern is whether overloading + and adding a throwing behaviour* would cause less confusion than adding a whole new syntax feature.

*(then again... are silent failures caused by strings + numbers really that enviable? Though that'd mean you'd have to stringify your records before string concatenation or interpolate them. That will trip up many devs, but would also encourage them to avoid blind string concatenation....)

Using the user.street.address example:

let newRec = myRec + #{ user: { street: { address: 'Foo Blvd' }}}

... a little more verbose but yeah that works well. Plus you also create the path if it doesn't already exist.

@ljharb
Copy link
Member

ljharb commented Aug 18, 2019

Symbols already require explicit stringification, so there’s precedent there.

@Alexsey
Copy link

Alexsey commented Aug 18, 2019

@ljharb why do you think that combination of mutable and immutable data structures is a bad idea? It should not be a problem to convert a mutable object to immutable analog, and in cases when it would be required developers will do it - there is no second scenario to follow

About someObject with .b = 5 syntax - maybe dot in construction <operator> .b should also become a separate syntax (proposal) beyond with use case? For example, I would be happy to do

const names = users.map(user => .name)
// or
const  trimmedLines = lines.map(line => .trim())

and there is a demand on such shorthands - for example lodash provides for this

const names = users.map('name')
// and
const trimmedLines = lines.invokeMap('trim')

but this alternatives uses string literals an thought are error prone

If we are talking about the operator that in some codebases would be used instead of = which means REALLY often - maybe it should be shorter than 4 symbols?

@ljharb
Copy link
Member

ljharb commented Aug 18, 2019

Implicit coercion is something worth avoiding, even if it’s easy.

Separately, one shouldn’t optimize for writing, but for reading - since the latter happens way more often than the former.

@mheiber
Copy link
Contributor

mheiber commented Aug 28, 2019

For what it's worth, my own personal opinion is that it would be good to see a PR for removing with from the current proposal, with the idea that it would make sense as a follow-on.

with has value, in my opinion, but ideally the design of with won't impede progress in achieving the core goals of the proposal.

@rricard
Copy link
Member

rricard commented Sep 10, 2019

See #49, we intend to merge this sometimes this week

@rricard
Copy link
Member

rricard commented Sep 10, 2019

with has been removed from the current proposal

@rricard rricard closed this as completed Sep 10, 2019
@Jack-Works
Copy link
Member

Then what about a immer.js like API?

Record.with(#{xyz: 1}, x => {
    x.xyz = 2
}) // return #{xyz: 2}

@rickbutton
Copy link
Member

rickbutton commented Mar 6, 2020

@Jack-Works I think Record.assign() in the namespace/prototype appendix fulfills that use case nicely:

Record.assign(#{ xyz: 1 }, #{ xyz: 2 }) === #{ xyz: 2 }

@Jack-Works
Copy link
Member

@Jack-Works I think Record.assign() in the namespace/prototype appendix fulfills that use case nicely:

Record.assign(#{ xyz: 1 }, #{ xyz: 2 }) === #{ xyz: 1 }

Does it assign the record deeply? Does it also work for tuple?

Record.assign(#{ xyz: #{abc: 1} }, #{ xyz: #{cde: 2} }) === #{ xyz: ??? }

@rickbutton
Copy link
Member

hmm, good question! As specified Record.assign is a shallow operation like Object.assign, but I think it would be feasible to describe a deeper version, ignoring unknown unknowns. Bikeshedding: Record.deepAssign?

@Jack-Works
Copy link
Member

Deep assign might be a footgun imo

It will be better to have a immer js like API . A Proxy like object is passed in, developer can modify it like an ordinary object, then after the modify, the proxy is revoked.

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

No branches or pull requests

9 participants