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

Backport ReadonlyRecord & Record #1455

Merged
merged 1 commit into from
Mar 31, 2021

Conversation

anthonyjoeseph
Copy link
Contributor

@anthonyjoeseph anthonyjoeseph commented Mar 28, 2021

Backports ReadonlyRecord from 3.0.0 based on the 2.11 roadmap #1446

I agree w/ the Ord idea outlined in the roadmap, but I can change if desired.

Notes:

  • I used Ord constraints in Record as well, I can change it to use Ord.trivial if desired
  • I wasn't sure why ReadonlyRecord.wilt and ReadonlyRecord.wither use Ord.trivial in 3.0.0, but I mirrored that - I'm not sure if that was correct
  • I also added Ord instances to these, which were unspecified - we can use Ord.trivial instead if desired
    • traverse
    • sequence
    • getShow
    • typeclass instances (Traversable -> getTraversable: (O: Ord<string>) => ...)

@gcanti
Copy link
Owner

gcanti commented Mar 29, 2021

I agree w/ the Ord idea outlined in the roadmap

We can't just change the signatures though, this PR contains the following breaking changes:

  • keys
  • collect
  • toReadonlyArray
  • toUnfoldable
  • reduceWithIndex
  • foldMapWithIndex
  • reduceRightWithIndex
  • traverseWithIndex
  • traverse
  • sequence
  • reduce
  • foldMap
  • reduceRight
  • getShow
  • Foldable -> getFoldable
  • FoldableWithIndex -> getFoldableWithIndex
  • Traversable -> getTraversable
  • TraversableWithIndex -> getTraversableWithIndex
  • Witherable -> getWitherable

I wasn't sure why ReadonlyRecord.wilt and ReadonlyRecord.wither...

This was a mistake, thanks for pointing out (fixed now in v3)

@anthonyjoeseph
Copy link
Contributor Author

anthonyjoeseph commented Mar 29, 2021

Oh, ok - I misunderstood the roadmap & the release process, my bad. Do you think it would make sense to add new functions w/ *WithOrd signatures (e.g. keysWithOrd)? Or maybe it's best to leave all that out for the moment?

@gcanti
Copy link
Owner

gcanti commented Mar 29, 2021

Yeah, I'm sorry, the roadmap wan't clear on this point, I put a "?" on the item list because I'm not even sure is possible to backport the new constraint.

In general we can do whatever we want in the 3.0.0 branch (with an eye on the migration path though) but in the 2.x series we can't introduce breaking changes.

In the past I used the following technique:

  • add a new signature as an overload to the API
  • deprecate the old signature

Let's pick keys for example:

export const keys = <K extends string>(r: ReadonlyRecord<K, unknown>): ReadonlyArray<K> =>
  (Object.keys(r) as any).sort()

The new definition would be something like:

declare const isOrd: (r: Ord<string> | ReadonlyRecord<string, unknown>) => r is Ord<string>

/**
 * @since 2.5.0
 */
export function keys(O: Ord<string>): <K extends string>(r: ReadonlyRecord<K, unknown>) => ReadonlyArray<K>
/**
 * Use the other overload instead
 *
 * @deprecated
 */
export function keys<K extends string>(r: ReadonlyRecord<K, unknown>): ReadonlyArray<K>
export function keys(
  or: Ord<string> | ReadonlyRecord<string, unknown>
): ((r: ReadonlyRecord<string, unknown>) => ReadonlyArray<string>) | ReadonlyArray<string> {
  return isOrd(or) ? keys(or) : (Object.keys(or) as any).sort()
}

The problem is the isOrd guard: how can we reliably distinguish between an Ord and a ReadonlyRecord?

@anthonyjoeseph
Copy link
Contributor Author

anthonyjoeseph commented Mar 29, 2021

No worries! Oh good question. Maybe something like this?

const isOrd = (a: Record<string, unknown> | Ord<string>): a is Ord<string> =>
  Object.keys(a).length === 2
  && 'equals' in a
  && typeof a.equals === 'function'
  && 'compare' in a
  && typeof a.compare === 'function'

I know it's super kludgy, but we could probably assume that would work

@gcanti
Copy link
Owner

gcanti commented Mar 29, 2021

I would:

  • keep as is:
    • keys (you can always sort the output, or, if you want the insertion order, use Object.keys)
    • toReadonlyArray (you can always sort the output, or, if you want the insertion order, use Object.values)
    • toUnfoldable, traverseWithIndex, traverse, sequence (it doesn't worth it)
  • apply the technique to:
    • collect (this first parameter is a function so we are safe)
    • reduceWithIndex, reduceRightWithIndex, reduce, reduceRight (checking the arguments length I guess)
    • foldMapWithIndex, foldMap, getShow (checking compare? ugly...)
  • add a new API and deprecate the old one (which we keep as is to not introduce breaking changes):
    • Foldable -> getFoldable
    • FoldableWithIndex -> getFoldableWithIndex
    • Traversable -> getTraversable
    • TraversableWithIndex -> getTraversableWithIndex
    • Witherable -> getWitherable

@gcanti
Copy link
Owner

gcanti commented Mar 29, 2021

I would...

btw I know this is an horrible task, so feel free to close this PR or just clean it up by keeping only the new additions like union, intersection, etc...

@anthonyjoeseph
Copy link
Contributor Author

Not at all - it's my pleasure! It should be good now.

By the way, I saw your comment in the Roadmap - please feel free to close this or steal from it if that'd be easier for you. Or I can remove the Ord stuff if that's easiest

@gcanti
Copy link
Owner

gcanti commented Mar 31, 2021

@anthonyjoeseph awesome, thank you!

@gcanti gcanti merged commit d6ccecb into gcanti:2.11 Mar 31, 2021
@anthonyjoeseph anthonyjoeseph deleted the 2.11-readonlyrecord branch March 31, 2021 12:49
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

Successfully merging this pull request may close these issues.

2 participants