Skip to content

Improved Flow support for Record subclasses #1414

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

Merged
merged 2 commits into from
Oct 20, 2017
Merged

Conversation

leebyron
Copy link
Collaborator

Not ideal, but this adds dramatically better support for subclassing Records when using Flow. The cost of this is that the default values passed to Record are no longer checked - they should be separately checked :/. I've updated the documentation to show this in the example, as well as including an example of subclassing a Record when using Flow.

Fixes #1388

Not ideal, but this adds dramatically better support for subclassing Records when using Flow. The cost of this is that the default values passed to Record are no longer checked - they should be separately checked :/. I've updated the documentation to show this in the example, as well as including an example of subclassing a Record when using Flow.

Fixes #1388
@leebyron leebyron merged commit d23746f into master Oct 20, 2017
@leebyron leebyron deleted the flow-subclass-record branch October 20, 2017 02:42
@acusti
Copy link
Contributor

acusti commented Oct 20, 2017

The subclassed example looks great: easy, simple, clear. Just to be sure I understand, the trade-off is that regular Records change from being typed like:

type Point3DProps = { x: number, y: number, z: number };
const makePoint3D: RecordFactory<Point3DProps> = Record({ x: 0, y: 0, z: 0 });

to:

type Point3DProps = { x: number, y: number, z: number };
const defaultValues: Point3DProps = { x: 0, y: 0, z: 0 };
const makePoint3D: RecordFactory<Point3DProps> = Record(defaultValues);

where the default values are defined separately and typed at the point that it is defined? If so, it seems like a reasonable trade-off to me.

@leebyron
Copy link
Collaborator Author

leebyron commented Oct 20, 2017 via email

@patriq
Copy link

patriq commented Nov 6, 2017

How can I use this in my project right now?
Tried using "immutable": "git://github.com/facebook/immutable-js.git#d23746f" as a dependency but since there is no compiled version in the github repository it simply can't find the module.
Any suggestion to use this with a team of developers, without me telling me what do change, or compile the module myself?

@acusti
Copy link
Contributor

acusti commented Nov 7, 2017

@patriqdesigns Here’s what I’ve been using while we wait for the stable 4.0.0:

    "immutable": "facebook/immutable-js#npm",

This gives you the latest build for npm always. It reflects whatever is in the master branch, and is the most bleeding edge thing you can use. You can also choose a specific pre-release version, the latest being 4.0.0-rc.9:

    "immutable": "^4.0.0-rc.9",

@patriq
Copy link

patriq commented Nov 7, 2017

Thanks man! Should have looked into the branches instead of trying to use the specific commit...

@alaindresse
Copy link

I would love to use this, but am a bit held off because I don't know how close we are to a stable 4.0.0.

We don't use immutablejs yet in the current project, and will introduce it, within a limited scope which will be deployed in the coming 4 weeks. Would you recommend I stick to the latest stable version ?

@acusti
Copy link
Contributor

acusti commented Nov 9, 2017

@alaindresse Do you use flow or typescript? If so, the v4 release candidates are a massive improvement over v3.x. However, there are known issues, and I don’t have any idea about the timeline for stable v4 being released, so if you aren’t using flow or typescript, you should probably start with latest stable and only upgrade once 4.0.0 is released.

@alaindresse
Copy link

Thanks for the quick reply @acusti! I am indeed using Flow. Where can I see the known issues ?

@acusti
Copy link
Contributor

acusti commented Nov 9, 2017

You can find them in the open issues that explicitly mention immutable 4. They’re all on the first page of issues, and most of them are about types. But there are also a few about functionality, like #1438 and #1437 (which looks like an undocumented breaking change, but the undocumented part is the rub). Also note that the official docs at http://facebook.github.io/immutable-js/ describe the behavior of the latest release candidate (4.0.0-rc.9 right now), rather than the latest stable version.

@alaindresse
Copy link

Thanks. I had noticed the point on the official docs, which led me to consider using it for my project.

errendir added a commit to errendir/immutable-js that referenced this pull request Dec 8, 2017
* facebook/master: (305 commits)
  Reduce repetition in COC link `CONTRIBUTING.md`
  Add CODE_OF_CONDUCT.md
  fix typo
  Add link to Code of Conduct in CONTRIBUTING
  Improved Flow support for Record subclasses (immutable-js#1414)
  Improve asImmutable() docs (immutable-js#1415)
  Only include version in require statement in Runkit instances. (immutable-js#1411)
  Add example for OrderedSet subtract. (immutable-js#1410)
  Fix code samples in Immutable.d.ts docs (immutable-js#1408)
  Adds perf tests for `Map.merge`. (immutable-js#1407)
  Move gulpfile into resources/
  Prettier: perf tests
  Prettier: include resources
  Prettify: Use es5 trailing commas
  Merge immutable-js#1403
  Document regression test steps. (immutable-js#1405)
  Demonstrates correct handling of Symbol keys in Map.mergeDeep when the Maps are nested, and initialized with Symbol KV tuples instead of plain JS object literals. (immutable-js#1404)
  Fix Map#concat (immutable-js#1402)
  test(ts-definitions): test Immutable.d.ts validity using tsc (immutable-js#1401)
  4.0.0-rc.9
  ...
errendir added a commit to errendir/immutable-js that referenced this pull request Dec 8, 2017
* facebook/master: (305 commits)
  Reduce repetition in COC link `CONTRIBUTING.md`
  Add CODE_OF_CONDUCT.md
  fix typo
  Add link to Code of Conduct in CONTRIBUTING
  Improved Flow support for Record subclasses (immutable-js#1414)
  Improve asImmutable() docs (immutable-js#1415)
  Only include version in require statement in Runkit instances. (immutable-js#1411)
  Add example for OrderedSet subtract. (immutable-js#1410)
  Fix code samples in Immutable.d.ts docs (immutable-js#1408)
  Adds perf tests for `Map.merge`. (immutable-js#1407)
  Move gulpfile into resources/
  Prettier: perf tests
  Prettier: include resources
  Prettify: Use es5 trailing commas
  Merge immutable-js#1403
  Document regression test steps. (immutable-js#1405)
  Demonstrates correct handling of Symbol keys in Map.mergeDeep when the Maps are nested, and initialized with Symbol KV tuples instead of plain JS object literals. (immutable-js#1404)
  Fix Map#concat (immutable-js#1402)
  test(ts-definitions): test Immutable.d.ts validity using tsc (immutable-js#1401)
  4.0.0-rc.9
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants