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

README rework #169

Merged
merged 23 commits into from
Jul 19, 2020
Merged

README rework #169

merged 23 commits into from
Jul 19, 2020

Conversation

rricard
Copy link
Member

@rricard rricard commented Jul 18, 2020

The readme grew organically and forgot on the way to be a good intro to the proposal. This attempts to fix that.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Robin Ricard and others added 3 commits July 19, 2020 09:59
Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>
@rricard rricard marked this pull request as ready for review July 19, 2020 10:25
@rricard rricard changed the title wip: reworking the readme README rework Jul 19, 2020
Copy link
Member

@chicoxyzzy chicoxyzzy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice update!

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Co-authored-by: Sergey Rubanov <chi187@gmail.com>
Copy link
Member

@littledan littledan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's excellent to have these additional examples. It'd be good if we can iterate on them a bit more to be more convincing/practical-feeling, showing the benefits of immutability, and have different examples for Record vs Tuple. In general, there's still more to improve in terms of explaining the motivation; comments inline.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
details.md Outdated Show resolved Hide resolved
details.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated
@@ -532,6 +532,41 @@ Record & Tuple is built to interoperate with objects and arrays well: you can re

Developers used to manipulating objects in an immutable manner (such as transforming pieces of Redux state) will be able to continue to do the same manipulations they used to do on objects and arrays, this time, with more guarantees.

## Why are Record & Tuple not based on `.get()`/`.set()` methods like [Immutable.js](https://immutable-js.github.io/immutable-js/)?

If we want to keep access to Record & Tuple similar to Objects and Arrays as described in the previous section, we can't rely on methods to perform that access. Doing so would require us to branch code when trying to create a "generic" function able to take Objects/Arrays/Records/Tuples. We also want to avoid an ecoystem split where libraries choose which structures they want to operate on.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the items proposal, this would no longer be true for Arrays/Tuples.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, that would make it possible for that operation to be generic. It does seem impossible to fix that with objects as adding accessor methods would possibly clash with keys.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which proposal are you referring to? The item method?

Even if some APIs to access Tuples vs Arrays are generic, this proposal aims to have greater ergonomics by going for maximal compatibility. I hope this simplifies the mental model for programmers, to minimize the number of differences they have to remember.

@rricard
Copy link
Member Author

rricard commented Jul 19, 2020

merging those changes for now, will need to add a few other PRs to finish the rationale properly

@rricard rricard merged commit 6b4b1d9 into master Jul 19, 2020
@rricard rricard deleted the readme/rework branch July 19, 2020 16:11
README.md Outdated

Likewise Tuples in Typescript are a notation to express types in an array of a limited size (starting with TypeScript 4.0 they have a [variadic form](https://github.com/microsoft/TypeScript/pull/39094)).

TS Records or Tuples could end up be usable on ECMAScript Records and Tuples as well, for instance:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: being

README.md Outdated

```ts
// potential TS Record of an ECMAScript Record
const record: RecordRecord<string, number> = #{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: RecordRecord

README.md Outdated
};

// potential TS Tuple of an ECMAScript Tuple
const tuple: #[number, string] = #[1, "foo"];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're trying to show Tuples with today's TypeScript, so we should drop the first type annotation # character.

If TS had support for Tuples as types, the whole annotation would be redundant.

@rricard
Copy link
Member Author

rricard commented Jul 19, 2020

thanks for the feedback rob but I already had that refactored prior to merging, let me know if the new version needs refactoring

rickbutton pushed a commit that referenced this pull request Jan 4, 2021
* wip: reworking the readme

* tuple examples

* typo

* extend the why not objects section

* clarify TS records and tuples

* more rationale

* Apply suggestions from code review

Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>

* expand the boxing section

* addition

* Apply suggestions from code review

Co-authored-by: Sergey Rubanov <chi187@gmail.com>

* Additional rational section

* comment

* better rename

* remove useless line

* compl changes to dan's suggestions

* Apply suggestions from code review

Co-authored-by: Daniel Ehrenberg <littledan@igalia.com>

* change tuple example

* adding that interning is slow

* add research

* clarify TS

* nit

* redo get/set example

* typo

Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>
Co-authored-by: Sergey Rubanov <chi187@gmail.com>
Co-authored-by: Daniel Ehrenberg <littledan@igalia.com>
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.

6 participants