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

Further improve stringToPath types; consolidate setPath and pathOr? #632

Open
Brodzko opened this issue Apr 11, 2024 · 5 comments
Open

Further improve stringToPath types; consolidate setPath and pathOr? #632

Brodzko opened this issue Apr 11, 2024 · 5 comments
Labels
feature request New feature or request runtime Issues that refer to the runtime behavior

Comments

@Brodzko
Copy link
Contributor

Brodzko commented Apr 11, 2024

As discussed in #630 (review), it doesn't seem to make sense to support "dotted paths nested within bracket access", as e.g. foo[bar.baz] is not even a valid object path.

The suggestion is to only allow integers inside brackets and also parse them out as numbers, as this is the format setPath expects.

I'd agree with @cjquines that this doesn't need to be considered a breaking change, because the current version seems more like a bug than an intention (and also seems kind of useless in those specific cases).

On the other hand, if someone is relying on numeric indexes being parsed as strings instead of numbers, things could break.

Another question is whether pathOr shouldn't be brought "on par" with setPath, meaning supporting the same format of inputs, including at least somewhat arbitrary depths.

Related: #355

@Brodzko
Copy link
Contributor Author

Brodzko commented Apr 11, 2024

Could also be worth it to explicitly state in documentation, that keys containing dots are not considered valid keys from remedas POV, even though they are technically legal in Javascript? Personally, I find that to be a reasonable convention, in my years of doing frontend, I don't think I have seen an object use dots in key names 🤔

@cjquines cjquines added feature request New feature or request runtime Issues that refer to the runtime behavior labels Apr 16, 2024
@Brodzko
Copy link
Contributor Author

Brodzko commented Apr 19, 2024

Been thinking about this for a while and I think before we do anything, we should agree on a proper spec; so here is a rundown of some things I thought about and my proposals/RFCs - I'd like to get some feedback and discuss this, identify potential breaking changes and decide how to go about implementation, if you want to go forward with this. Let's start with

stringToPath

Should convert a valid "dotted path" into a valid "tupled" path, preserving type safety.

The first interesting question to ask is "what is a valid dotted path"? We all know the simple "foo.bar", "foo[0].bar". But dots are valid characters in object paths in JS (and JSON), so what would be a representation an object like this?

const whatNow = {
  foo : {
    "bar.baz": 42
  }
}

One option would be to decide we simply don't want to support objects like this, but Murphy's law tells us sooner or later this would become a problem for some users. While a definite edge-case, it is solvable by using a special delimiter for keys like these. An obvious candidate would be quotes/double quotes, so a correct representation would be

const path = "foo.'bar.baz'";
// or
const path = 'foo."bar.baz"';

The complication is we would need to support both so it does not depend on people's quoting preferences.

When it comes to array types, I believe it makes sense to use numbers for index access, so for

const objWithArray = {
   foo: [{ bar: 42 }]
}

we would use

const path = ["foo", 0, "bar"];

This also distinguishes from objects with numeric keys (another edge-but-valid case), so for

const weirdObj = {
   42: {
      foo: "tada"
    }
}

the path would actually be

const path = ["42", "foo"];

TLDR;

  • use strings for interpreting object keys (even numeric ones)
  • support keys with dots by using quotes as delimiters in path
  • use number for index access to arrays

WDYT @cjquines ?

@Brodzko
Copy link
Contributor Author

Brodzko commented Apr 19, 2024

pathOr

Should be able to retrieve a value at path conforming to rules defined above in stringToPath (including keys containing dots and array index accesses).

I support the proposal of defaultValue being allowed to be of different type than type at specified path. This is useful, because sometimes you genuinely want to fall-back to at least nullish, even if in the type definition the type is not nullable.

This would essentially mean changing the type signature to something like

function pathOr = <T Path extends PathOf<T>, TDefault>(obj: T, path: Path, defaultValue: TDefault): TypeAt<T, Path> | TDefault

T above could be a Record or an Array, so even const thirdElement = R.pathOr([1, 2, 3], [2], 42); // 3 would be valid - useful in particular for composing accessors to deep objects that contain arrays.

Would be awesome if pathOr could support deeper paths than the current 3 (say at least 10). I'm sure this is implementable, but not sure how much strain it puts on the type checker and what's the best we can do in terms of optimization.

Another issue with typing is non-homogenous types, e.g. unions. Consider

type Food = {
  kind: 'food';
  food: string;
}

type Drink = {
  kind: 'drink';
  drink: string;
}

const foodOrDrink: Food | Drink = {...};

const result = R.pathOr(foodOrDrink, ['drink'], 'coke'); // currently invalid

This is currently not possible, because drink is not considered a valid path in result. Actually, even asking for

const result = R.pathOr(foodOrDrink, ['kind'], 'drink'); // breaks

fails because pathOr simply isn't capable of considering union types. It gets even trickier if one of the union constituents has nullable/optional properties, e.g. for

type Foo = {
  bar?: {
    baz: string;
  } | null;
}

even Typescript cannot do Foo['bar']['baz'] (understandably so). However, for a function that is prepared to not find a value at the path, I'd argue this should actually be possible.

Now I believe this would be tremendously useful, especially in established code-bases that have less-than-ideal typings.lodash's get can do all of this, the only problem being it virtually always returns any all over the place. The goal would be to provide a true replacement for get with all of the type-safety.

The question is how to go about it. Maybe take all possible constituent types, make them deeply required and non-nullable and collect all the paths? And making the resulting value optional/nullable if there is a potentially non-existing key/index access on the way? Again, not sure what the performance implication would be type-checker-wise.

Note that this would mean that pathOr would work kind of like a getter of a prism?

@Brodzko
Copy link
Contributor Author

Brodzko commented Apr 19, 2024

setPath

Should immutably set value at path conforming to rules defined above in stringToPath (including keys containing dots and array index accesses).

Essentially everything said for pathOr applies here, with an additional problem of:

  • what to do if while traversing the path we hit a non-existent object? Do we recursively create the object? Do nothing? Throw?
  • what if the value at path is an array? Replacing the entire array without having access to previous value is not ideal.
  • what if the path ends in array access, e.g. ['foo', 'bar', 42]? Especially if the array does not contain the "previous" elements? Do we push to the end of it? Do nothing? Create sparse arrays? Throw?

These I believe are tricky to define reasonably, but would also be super handy if we can do it right.

@cjquines
Copy link
Collaborator

stringToPath:

support keys with dots by using quotes as delimiters in path

i doubt it's worth the effort supporting this on a typing-level, and it'd be a backwards-compatible change if we do decide to support it in the future. let's hold off on this for now and add it if someone requests

pathOr: i'm going to repeat some of the points in #355... i don't think we should spend too much effort making pathOr work in all cases. the main value of pathOr is with stringToPath with strings we don't know compile-time. otherwise we'd prefer optional chaining. in this case i don't think there's much we can get out of pathOr anyway

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request runtime Issues that refer to the runtime behavior
Projects
None yet
Development

No branches or pull requests

2 participants