Flow errors with flow 0.90.0 and draft-js-js 0.10.5

[yiblet]

Do you want to request a feature or report a bug? report a bug What is the current behavior? Flow shows 25 different errors when typechecking draft js

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. You can use this jsfiddle to get started: https://jsfiddle.net/stopachka/m6z0xn4r/.

Here's the output when I run flow in the lib/ directory

What is the expected behavior? flow shouldn't throw errors

Which versions of Draft.js, and which browser / OS are affected by this issue? Did this work in previous versions of Draft.js?

flow 0.90.0 & draft js 0.10.5

[mrkev]

Gotcha. This will be fixed on the next release. Thanks for pointing it out! 👍

[yiblet]

Thanks! do you have an idea when that release will be out?

[niveditc]

We're currently working on cutting down the size of the release, which is the main blocker for a new release. If our efforts are successful, should be fairly soon :)

cc @claudiopro who is working on this

[claudiopro]

hi @yiblet, we're running flow 0.92 on master and as @mrkev said, it passes without errors. We'll keep contributors and users updated about the next release. Please track #1312 for updates.

[daryn-k]

Any news? Because flow doesn't work correctly without ignore node_modules. With node_modulesflow throws errors because of Draft-js.

[mrkev]

This will be fixed in the release of 0.11. We're in a bit of a difficult situation here, since it's been a while since we an official release is made, so we wan't to minimize the risk of releasing. There's two main fronts:

• Are there breaking changes in the API?
• Are there performance regressions?

Last I checked, we found a couple of issues on the latter. I'll investigate further into mitigating that issue as well as we can and getting 0.11 out the door sooner rather than later— I'm personally just ramping into some busy times in my life (big move, changing cities) so I can't say I'll have much time to commit to this myself until May. Updates around the release will probably come in #1312 though.

[thibaudcolas]

I'm still getting Flow errors in the Draft.js code with the v0.11.0-beta2 (& Flow v0.91.0), consistently across the 3 projects where I use Draft.js and Flow. Is there something I'm missing?

Here are the errors for reference:

Flow errors

``` Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ node_modules/draft-js/lib/DraftEditor.react.js.flow:344:22 Property role is missing in DraftEditorProps [1]. [1] 158│ class DraftEditor extends React.Component { : 341│ * found when Flow v0.68 was deployed. To see the error delete this comment 342│ * and run Flow. */ 343│ 344│ const ariaRole = this.props.role || 'textbox'; 345│ const ariaExpanded = ariaRole === 'combobox' ? !!this.props.ariaExpanded : null; 346│ const editorContentsProps = { 347│ blockRenderMap, Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ node_modules/draft-js/lib/DraftEditorDragHandler.js.flow:45:14 Property caretRangeFromPoint is missing in Document [1]. node_modules/draft-js/lib/DraftEditorDragHandler.js.flow 42│ * found when Flow v0.68 was deployed. To see the error delete this comment 43│ * and run Flow. */ 44│ 45│ if (typeof document.caretRangeFromPoint === 'function') { 46│ const dropRange = document.caretRangeFromPoint(event.x, event.y); 47│ node = dropRange.startContainer; 48│ offset = dropRange.startOffset; /private/tmp/flow/flowlib_2a2916c9/dom.js [1] 1205│ declare var document: Document; Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ node_modules/draft-js/lib/DraftEditorDragHandler.js.flow:46:32 Cannot call document.caretRangeFromPoint because the parameter types of an unknown function [1] are unknown. 43│ * and run Flow. */ 44│ [1] 45│ if (typeof document.caretRangeFromPoint === 'function') { 46│ const dropRange = document.caretRangeFromPoint(event.x, event.y); 47│ node = dropRange.startContainer; 48│ offset = dropRange.startOffset; 49│ } else if (event.rangeParent) { Error ┈┈┈┈┈┈┈┈┈┈ node_modules/draft-js/lib/convertFromHTMLToContentBlocks.js.flow:680:26 Property blockType is missing in ContentBlockConfig [1]. [1] 664│ _extractTextFromBlockConfigs(blockConfigs: Array): { : 677│ * found when Flow v0.68 was deployed. To see the error delete this 678│ * comment and run Flow. */ 679│ 680│ if (text !== '' && config.blockType !== 'unstyled') { 681│ text += '\n'; 682│ characterList = characterList.push(characterList.last()); 683│ } ```

I think I was getting all of those errors before, except for node_modules/draft-js/lib/convertFromHTMLToContentBlocks.js.flow:680:26 which seems to be new.

[fravic]

Flow errors I think I was getting all of those errors before, except for node_modules/draft-js/lib/convertFromHTMLToContentBlocks.js.flow:680:26 which seems to be new.

These flow errors seem to result from a newline being inserted after the $FlowFixMe block comments somewhere in the gulp pipeline (something in babel-preset-fbjs)? I tried to hunt down what was inserting the newline with no success.

[thibaudcolas]

Ah, nice catch! Looking further at the specific errors, they all come from the same commit for the Flow v0.68 upgrade https://github.com/facebook/draft-js/commit/a99f51eb33dc7c529f21b87d6cc7394b073f9388. I wouldn't be surprised if it was easier to fix them rather than ignore them and change the build pipeline.

Property role is missing in DraftEditorProps [1]

https://github.com/facebook/draft-js/blob/d09ef3e416beb766b5a227ea84e8d735ff11823b/src/component/base/DraftEditor.react.js#L335-L340

^ unless I'm missing something, this is just a matter of adding the property to https://github.com/facebook/draft-js/blob/master/src/component/base/DraftEditorProps.js.

Property caretRangeFromPoint is missing in Document [1]

https://github.com/facebook/draft-js/blob/a99f51eb33dc7c529f21b87d6cc7394b073f9388/src/component/handlers/drag/DraftEditorDragHandler.js#L39-L46

This one looks slightly trickier. Related Flow issue: https://github.com/facebook/flow/issues/6937. Seems like a potential solution would be to do what's suggested in https://github.com/facebook/flow/issues/396#issuecomment-400882217,

declare interface Document extends Document {
  // TODO Use the correct signature.
  caretRangeFromPoint: () => {},
}

Property blockType is missing in ContentBlockConfig [1]

https://github.com/facebook/draft-js/blob/a99f51eb33dc7c529f21b87d6cc7394b073f9388/src/model/encoding/convertFromHTMLToContentBlocks2.js#L667-L670

^ just need to add the property to the type definition? https://github.com/facebook/draft-js/blob/a99f51eb33dc7c529f21b87d6cc7394b073f9388/src/model/encoding/convertFromHTMLToContentBlocks2.js#L164-L176

---

If there is some agreement that this is the correct approach here I'm happy to make a PR with the fixes.

For the caretRangeFromPoint error, seeing https://github.com/facebook/flow/issues/6937#issuecomment-425126472, someone at Facebook might have more thoughts on what the proper fix for this would be – whether Flow should have built-in support for non-standard DOM APIs that are established, or whether this is better done as an override here, or whether this project shouldn't use non-standard APIs to start with.

[claudiopro]

Thanks for reporting this issue @yiblet! Let me look into this @thibaudcolas.

[mrkev]

@thread is this still an issue?

[malectro]

Given the issues with babel, would using suppression_type instead of suppression_comment work better? Some of these suppression comments are necessary because Flow doesn't support bleeding-edge browser APIs.

example:

/* $FlowFixMe(>=0.122.0 site=www) This comment suppresses something. */
callFunction(thing);

becomes

// FixMe(>=0.122.0 site=www) We suppress something here because of reasons.
callFunction((thing: $FlowFixMe);

[mrkev]

@malectro suppression comments are generally preferred.

I just ran flow on the repo and see no errors. Closing this issue.

[thibaudcolas]

@mrkev at least for me the problem is with projects I’m using Draft.js and Flow on (https://github.com/facebook/draft-js/issues/1974#issuecomment-479681857), not the Draft.js repo itself. Could we leave this issue open on that basis, unless that’s fixed too? Or is there another issue for that?

I’m not super up-to-date with this but last time I checked I needed the following overrides to disable Flow checks for Draft.js code, which somewhat defeats the purpose of Draft.js publishing flow type definitions to start with:

https://github.com/thibaudcolas/draftjs-filters/blob/f2259e1dcff93f7ebfb7664cca93f8a714e6de3b/.flowconfig#L1-L10

[ignore]
.*/node_modules/draft-js/lib/DraftEditorContents.react.js.flow
.*/node_modules/draft-js/lib/ContentBlockNode.js.flow
.*/node_modules/draft-js/lib/ContentBlock.js.flow
.*/node_modules/draft-js/lib/DraftEditorLeaf.react.js.flow
.*/node_modules/draft-js/lib/DraftEditorDragHandler.js.flow
.*/node_modules/draft-js/lib/DraftEditor.react.js.flow
.*/node_modules/config-chain
.*/node_modules/fbjs/lib/emptyFunction.js.flow

[mrkev]

@thibaudcolas yeah, let's open a new issue for that. Include the info in both your comments too 👍

[malectro]

hey @mrkev i understand suppression comments are preferred, but isn't the issue this?

These flow errors seem to result from a newline being inserted after the $FlowFixMe block comments somewhere in the gulp pipeline (something in babel-preset-fbjs)? I tried to hunt down what was inserting the newline with no success.

running Flow on the Draft code base isn't going to yield any errors because the errors only pop up in the built files. if you instead run node_modules/.bin/flow --flowconfig-name src/.flowconfig lib you'll get quite a few.

as an example, src/model/immutable/SelectionState.js has a suppression comment on L35 that allows for the use of any on L38, but in lib/SelectionState.js.flow, the suppression comment is moved to L34 and has no affect. if the resulting .flow files in the lib folder do not pass a Flow check, it does seem to be an issue with the Draft repo itself.

[mrkev]

Oh, I didn't notice this, sorry.

Hmm, the build shouldn't be doing this, because comments are the preferred way of suppressing errors. Error codes are coming to Flow soon. FlowFixMe's are inherently unsafe because is something else breaks on a FlowFixMe'd line, Flow won't catch it. Error codes will provide a way to narrow the suppression space and reduce the unsafety of FlowFixMe's, and afaik it will work with comments, not the suppression type.

We should look into fixing the build here.

[malectro]

No problem. Thanks for repoening.

This is a bummer because it doesn't look like Babel outputs a consistent position for suppression comments, and this comment implies they have no plans to change things https://github.com/babel/babel/issues/5512#issuecomment-372387071

Without using a suppression type it seems like there are two options:

• Fix the Flow issues
  • This would be an ongoing, difficult project.
• Somehow make the original source files available in the package instead of the transpiled files?
  • This would mean dependents would have to be able to parse all the extra syntax included in preset-fbjs.