Using flow, Record#set and Record#merge return intersection types (v4) #1388
Comments
|
These intersection types are a feature of v4 to get much better Record typing, though unfortunately there are still tradeoffs. One of those tradeoffs is that Record subclassing is not very well supported. Essentially every new Record factory is a constructor of a subclass of RecordInstance, but Flow does not have a way to dynamically create subclass types, so the work around is this intersection type which emulates a subclass. Typically these Record subclasses are just collections of methods, one strategy for updating them might be to convert the subclass into just a collection of functions which takes a Record as a first argument. Though I know that could be a really invasive change to Draft.js which heavily relies on record methods. I'm definitely open to suggestions for improving the types for Record subclassing if you have ideas |
|
I've asked @calebmer to help figure out a way to improve this, since I understand subclassing is a really common existing pattern. So far we've realized that the |
|
One work around I've figured out is to override the problem methods directly to the subclass: class ContentState extends ContentStateRecord {
set(k, v): this {
return (super.set(k, v): any);
}
}The root issue is that Flow misunderstands the type |
|
Delete comment to clear up issue. #1398 |
|
@athomann - hard to know without more information |
|
@athomann - that definitely looks like a typing bug, but I'm not certain what it is yet. Either way it's different from this one - would you mind opening a new issue? |
|
@leebyron I tried your workaround of overriding
var defaultRecord: {
key: string,
type: DraftBlockType,
text: string,
characterList: List<CharacterMetadata>,
depth: number,
data: Map<any, any>,
} = {
key: '',
type: 'unstyled',
text: '',
characterList: List(),
depth: 0,
data: Map(),
};
var ContentBlockRecord = Record(defaultRecord);
class ContentBlock extends ContentBlockRecord {
// Override problematic Record methods directly in the subclass
// https://github.com/facebook/immutable-js/issues/1388#issuecomment-337316952
set(k, v): this {
return super.set(k, v);
}
merge(obj: Object): this {
return super.merge(obj);
}
...
} |
|
Happy to see progress. Is there more to that error message? Flow's identifying that it's not Anothing thing we've noticed before is that Flow has trouble pinning down the type of values in the Record. We added some new utility types to help with this. I'm not sure if that's the issue at hand, but it might help: import type { RecordFactory } from 'immutable';
type ContentBlockFields = {
key: string,
type: DraftBlockType,
text: string,
characterList: List<CharacterMetadata>,
depth: number,
data: Map<any, any>,
};
var defaultRecord: ContentBlockFields = {
key: '',
type: 'unstyled',
text: '',
characterList: List(),
depth: 0,
data: Map(),
};
var ContentBlockRecord: RecordFactory<ContentBlockFields> = Record(defaultRecord);
class ContentBlock extends ContentBlockRecord {
// Override problematic Record methods directly in the subclass
// https://github.com/facebook/immutable-js/issues/1388#issuecomment-337316952
set(k, v): this & ContentBlockFields {
return super.set(k, v);
}
merge(obj: Object): this & ContentBlockFields {
return super.merge(obj);
}
...
} |
|
That’s the whole error message (no mention of But either way, good news! Using |
|
I spoke too soon. Making the above changes did greatly reduce the total error count, but flow still has all kinds of complaints about those subclassed Records. SelectionState.js looks like: type Fields = {
anchorKey: string,
anchorOffset: number,
focusKey: string,
focusOffset: number,
isBackward: boolean,
hasFocus: boolean,
};
var defaultRecord: Fields = {
anchorKey: '',
anchorOffset: 0,
focusKey: '',
focusOffset: 0,
isBackward: false,
hasFocus: false,
};
var SelectionStateRecord: RecordFactory<Fields> = Record(defaultRecord);
class SelectionState extends SelectionStateRecord {
// Override problematic Record methods directly in the subclass
// https://github.com/facebook/immutable-js/issues/1388#issuecomment-337316952
set(k: string, v: any): this & Fields {
return super.set(k, v);
}
merge(obj: Object): this & Fields {
return super.merge(obj);
}
...
} |
|
Yeah - this is the same error as before but now locallzed to your override fix. You can tell Flow to leave you alone since we're already doing silly things to work around it that we know are safe: set(k: string, v: any): this & Fields {
return (super.set(k, v): any);
} |
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
|
@acusti - I just put up a PR that should make this considerably easier after brainstorming with some folks on the Flow team. Take a look at the examples in there and let me know if they look reasonable to you |
* Improved Flow support for Record subclasses 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 * Update record.js
What happened
In trying to upgrade draft-js to
4.0.0-rc.7(and flow 0.57.1), I ran into many errors all around the fact that calling.set()or.merge()on a Record returns an intersection type that then causes errors for any other typing that refers to the original Record, or in the case of draft, that refers to a class that extends the Record, which is the common pattern in that codebase. For example:ContentState.js, the Record-based class:updateEntityDataInContentState.js:Which leads to:
Is there a way to cast the result of every
setandmergeoperation somehow to ensure that the value can still be typed as the original record type? Or any other recommended way to get around this issue?Also, @leebyron, is opening issues here a useful way for me to report issues I come across as I work through getting draft.js upgraded (facebookarchive/draft-js#1439)? @flarnie mentioned that you were open to suggestions or questions; if there’s a better venue for that, please let me know. Thanks!
The text was updated successfully, but these errors were encountered: