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

Union Types #12

Closed
hoeck opened this issue Nov 24, 2017 · 6 comments
Closed

Union Types #12

hoeck opened this issue Nov 24, 2017 · 6 comments

Comments

@hoeck
Copy link

hoeck commented Nov 24, 2017

Nice library! I fully agree with what you wrote in the README about "Why Object/Array instead of immutable data structures". I'd love to use it everywhere instead of deeply nested object splice expressions.

What keeps me from completely switching to deepUpdate though is that it does not deal well with union types. For example this:

import { deepUpdate } from 'immupdate'

interface A {
    name: 'a',
    foo: number,
}

interface B {
    name: 'b',
    bar: string,
}

interface Container {
    aOrB: A | B
}

const c: Container = {aOrB: {name: 'a', foo: 1}}

deepUpdate(c).at('aOrB').at('foo').set(2)

results in the following type error for me (using lots of strict flags in tsconfig and Typescript 2.6):

The 'this' context of type 'BoundUpdater<Container, A | B>' is not assignable to method's 'this' of type 'BoundAtUpdater<Container, any[]>'.
    Types of property '__T' are incompatible.
      Type '[Container, A | B]' is not assignable to type '[Container, any[]]'.
        Type 'A | B' is not assignable to type 'any[]'.
          Type 'A' is not assignable to type 'any[]'.
            Property 'includes' is missing in type 'A'.

I mean, raising a compilation error here totally makes sense because there are no typeguards anywhere around .at('foo').

Do you have any thoughts about how one could solve this without giving up too much of the deepUpdate convenience? Is it even possible to use typeguards and generics together in TS?

@AlexGalays
Copy link
Owner

Can't believe I never encountered this as I use unions everywhere :D

You are right, it should be easy to do with immupdate.

Right now you would have to do something like this :

deepUpdate(c)
  .at('aOrB')
  .modify(u=> u.name === 'a'
    ? update(u, { foo, 2 })
    : u
  )

But that looks a bit funny and isn't as readable/declarative.

What would you think about a filter function?

e.g

const isA = (u: A | B): u is A => u.name === 'a'

deepUpdate(c)
  .at('aOrB')
  .filter(isA) 
  .at('foo')
  .set(2)

It would get even easier when/if this get worked on: microsoft/TypeScript#5101

As you wouldn't even need the artificial function annotated with is A, but at least this seems pretty readable/maintainable imo.

What do you think?

@hoeck
Copy link
Author

hoeck commented Nov 24, 2017

Thanks for the fast response!

The .filter method looks good because that mirrors what I would have written with plain object spread, e.g

    if (c.aOrB.name === 'a') { // or isA(c.aOrB)
        return {
            ...c,
            aOrB: {
                ...c.aOrB,
                foo: 2,
            },
        }
    }

and it seems similar to how the .abortIfUndef works right now. Also, my actual usecase is one where the union type is not the end of the path to be updated like:

deepUpdate(data).at('key').at('keyContainingUnionType').filter(isThatType).at('key').set(...)

.filter in this case would keep the code nice and flat and I might already have a type guard function at hand anyway.

Maybe there is a better name than .filter as for me that suggests an Array .filter/.map/.reduce pipeline, for example guard, isA or abortIfNot, ... ?

@AlexGalays
Copy link
Owner

AlexGalays commented Nov 24, 2017

I quite like the name filter because you may use it for other things than guarding against a particular type union instance, like "only update if this intermediary key has this value"; sort of like an assertion, but keeping the declarative style and only browsing the (sometimes nullable) keys once.

In lib.d.ts, Array.prototype.filter actually has an extra definition to recognize type guards, so I thought it was quite similar !

abortIfNot is very descriptive though, I like it and abortIfUndef would just be a special case.

@hoeck
Copy link
Author

hoeck commented Nov 24, 2017

Oh wow I didn't know that

const a: Array<number | string> = [1, 'A']
a.filter((x: any) x is string => typeof x === 'string')[0].toLowerCase()

is actually possible in TS right now!

So yeah, I'm personally fine with calling it .filter too as it would serve a similar purpose in conjunction with a typeguard when filtering Arrays. I was maybe just afraid that it may confuse other people who have to read and work with what I'm writing.

Also I would like .abortUnless (borrowing from the clojure/lisp unless macro) because it sounds so nice but as a non-native speaker an unless is almost 10 times harder to grasp when written in code than a simple if not for a reason I don't understand :).

So to close this comment from my point of view:
+1 for .filter if you value consistency with Typescript
+1 for .abortIfNot if you value consistency with your own lib

@AlexGalays
Copy link
Owner

AlexGalays commented Nov 25, 2017

Should be good in version 1.1.7 :)

Went with abortIfNot !

@hoeck
Copy link
Author

hoeck commented Nov 27, 2017

Awesome work! Just did a quick test in my actual code with immupdate 1.1.7 and now I can use deepUpdate to replace a bunch of unsafe object spreads. You rock, thanks!

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