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

fix: add immerable Flow typedef #447

Merged
merged 1 commit into from Oct 22, 2019

Conversation

nobitagit
Copy link
Contributor

I noticed the Flow annotation for immerable was missing, so this PR introduces it. Some things to note though:

Before this fix

Given the following code:

// @flow

import { immerable } from "immer";

class A {}
A[immerable] = true

Flow produces this error:

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/symbol.js:3:10

Cannot import immerable because there is no immerable export in immer.

     1│ // @flow
     2│
     3│ import { immerable } from "immer";
     4│
     5│ class A {}
     6│ A[immerable] = true;

Also, this is a screenshot of my IDE:

Immerable-flow-0

After this fix

My IDE seems happier about it (type is Symbol and no error is shown on the import):

Immerable-flow-1

Interestingly, running Flow after adding the type annotation still produces an error, although a different one.

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/symbol.js:6:3

Cannot assign computed property using Symbol [1].

     src/symbol.js
      3│ import { immerable } from "immer";
      4│
      5│ class A {}
      6│ A[immerable] = true;
      7│

     node_modules/immer/dist/immer.js.flow
 [1] 80│ export var immerable: Symbol

This appears to be because Flow doesn't like Symbols as object keys as of now.

Rationale

While this fix won't make that snippet of code compile just yet, I think it's fair to surface the error to the user as clearly related to the shortcomings of Flow, not because of a missing definition in the lib.

Happy to hear your thoughts, change this PR or close it entirely if you think that things are fine as they are. Thanks!

@mweststrate
Copy link
Collaborator

I agree this is an improvement! A hackish solution / temporarily follow up would maybe to declare it as string instead. Not sure whether that is a good idea :-P.

@mweststrate mweststrate merged commit 09d37ce into immerjs:master Oct 22, 2019
@aleclarson
Copy link
Member

🎉 This PR is included in version 4.0.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@nobitagit nobitagit deleted the flow-typedef-immerable branch October 22, 2019 16:02
@nobitagit
Copy link
Contributor Author

nobitagit commented Oct 22, 2019

A hackish solution / temporarily follow up would maybe to declare it as string instead. Not sure whether that is a good idea!

I think that for the moment this is fine as is. This is a valid use case for a symbol, and the error can just be suppressed in userland:

// $FlowFixMe see: https://github.com/facebook/flow/issues/3258
A[immerable] = true;

AFAICS, the user is not really expected to tamper with immerable, so I think it's an acceptable stopgap for now while Flow catches up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants