Stronger react dom refreshed #7569

villesau · 2019-03-18T20:59:20Z

This is revived #6727 (cc. @motiz88 ) so basically just test cases updated. I don't think it makes sense to cover everything in this PR (missing todos from the original pull request). Relevant docs (copied over from previous PR):

DOMProperty.js in react-dom
react-dom event system implementation and docs
The HTML living standard and MDN, naturally

Some insights from running this against our code base:

Should ReactDOM$GlobalEventHandlers functions return type be any/mixed instead of void?
- e.g async functions always returns promise.
- currently it's void (e.g onAbort?: (SyntheticEvent<E>) => void,) but does it matter what the function returns?
onChange is SyntheticEvent, should it be SyntheticInputEvent?
- onChange?: (SyntheticEvent<E>) => void,
dangerouslySetInnerHTML expects string, should it be optional ?string?
- We often pass nullable values. Should these be fixed or is it fine?
- dangerouslySetInnerHTML?: { __html: string },
and textArea onChange event.target.value is missing from event.target, should it?
- example: onChange={event => this.props.onChange({value: event.target.value})} <- event.target.value missing
- +target: EventTarget;
~~className only takes string or undefined, should it also allow null?~~
- ~~example: className={listStyle ? null : tableScrollWrapper~~
- ~~className?: string,~~
- E: Now allows null. Can be made stricter later if deemed necessary.
onClick only allows function. Should it also allow falsy values?
- example: onClick={busy ? null : onClick}

The biggest cause of issues for us is inputs returning SyntheticEvent instead of SyntheticInputEvent

goodmind · 2019-03-18T21:57:57Z

@villesau there shouldn't be value on event.target, use event.currentTarget

I don't think everything should allow null/undefined/falsey values. This is stricter types for reason?

villesau · 2019-04-01T19:38:36Z

@panagosg7 any news or comments about my findings? I wouldn't want to implement anything extra (something that might be lacking etc.) in this, bout I'm OK to fix the potential problems!

facebook-github-bot

@panagosg7 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

villesau · 2019-04-10T21:05:56Z

@panagosg7 has anyone had time to take a look at it or think about the findings from the first message? Would it make sense to weight in someone from React team to provide context?

panagosg7

Hi @villesau ! Thanks for all this work!

After some experimentation, I've added some comments for consideration. Would you be able to address these?

panagosg7 · 2019-04-10T22:15:56Z

lib/react-dom.js

+  onCanPlayCapture?: (SyntheticEvent<E>) => void,
+  onCanPlayThrough?: (SyntheticEvent<E>) => void,
+  onCanPlayThroughCapture?: (SyntheticEvent<E>) => void,
+  onChange?: (SyntheticEvent<E>) => void,


SyntheticEvent has a target property of type EventTarget which is missing value. This, however, seems to be common in examples. See https://reactjs.org/docs/forms.html#handling-multiple-inputs

Same holds for properties like checked, and less frequently for files, id, `blur, etc.

This probably has to do with the typing of target in SyntheticEvent:

https://github.com/facebook/flow/pull/7569/files#diff-ce626ccb6b8eb922c17388db8d33cbabR137

target is fine as is, currentTarget should be used

currentTarget should be used

Not on input or select elements, where we most likely don't need/want event propagation.

I usually type onChange handlers as onChange?: (SyntheticInputEvent<>) => void.
Does it means it will break when this lands?

Do we have some sort of consensus here? :D

@pascalduez, what you mean by event propagation? it IS target that propagates events

@panagosg7 @pascalduez @goodmind so what should this type actually be?

@panagosg7, @goodmind pointed to this in Discord: DefinitelyTyped/DefinitelyTyped#11508 (comment)

Does this make sense to you?

lib/react-dom.js

MichaReiser · 2019-04-22T20:16:54Z

Hi everyone. I stumbled upon this PR because I was interested in how event handlers in react should be typed (returning void or mixed?).

I looked at some other PR's and it seems that flow allows mixed return type for React lifecycle methods as well as DOM event handlers. Might it, therefore, make sense to type the event handlers with mixed as return type since it doesn't matter for react whatever is returned? This would also reduce the amount of breaking changes introduced by this PR since all existing callbacks will have the right return type.

This is a first pass to start surfacing the architectural and practical issues involved in typing React DOM more precisely. In this commit: - No event props yet! - React global attributes like dangerouslySetInnerHTML - Shared HTML/SVG attributes - HTML global attributes - A handful of SVG global attributes - Lots of things marked TODO - Some design choices described in comments "Booleanish string" as a term is lifted from the React DOM source.

This is based on facebook/react#12507 which is seemingly missing a couple of fields from the official spec. Consequently this will need an update once React supports all fields.

This also refactors the main set of JSX intrinsics declarations to support typing Event.currentTarget with each element's instance type.

This is a design choice documented in facebook#6727 (comment)

villesau · 2019-05-07T21:15:51Z

@panagosg7 addressed all the comments except the one which has some confusion around it. Would you also mind commenting the points I made in the PR description? Those are what I found out when testing this against our own code bases.

goodmind · 2019-05-20T12:39:22Z

I would love to see SyntheticEvent exported in react aka import {SyntheticEvent} from 'react' and not global

facebook-github-bot

@panagosg7 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

villesau · 2019-06-07T20:05:10Z

ping @panagosg7 any news?

panagosg7 · 2019-06-07T20:56:57Z

Sorry for the delay. I tried these changes internally, but I'm afraid the overhead is more than we can afford at this point. In addition to the error diff, which we could probably work around, the merge time increased by more than 20%. This makes it really hard to move forward with this.

villesau · 2019-06-07T21:49:43Z

@panagosg7 what do you mean by merge time?

This obviously is a big change (and a big improvement as well): Right now React is totally untyped around this area which is super risky and bug prone. Not only once or twice types around this would have caught bugs that we had later noticed too late.

In addition to added safety this would impact dev experience since after this Flow would start to provide better auto complete for plenty of React specific functions I believe. In overall I'm quite confident this would make developers more confident with Flow.

TS is currently doing very well around this area: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/react/index.d.ts#L2499

panagosg7 · 2019-06-07T23:01:52Z

By merge time I mean the main Flow's main checking procedure.

I am sold with and very appreciative of the improvement in checking that this offers, but we've invested about a year in performance gains, so have to be extra careful in making sure we deliver on that account. (Realistically, even if I landed this change as is, it would possibly get reverted come release time.)

In the meantime, until we figure out why this lib change causes this perf regression, can these typings be imported to your project so that you can maintain the checking benefits? (In TS these typings seem to live in Definitely typed rather than the main TS repo.)

goodmind · 2019-06-08T03:14:38Z

@panagosg7 this would require per-lib ignore as in

#7732, so that we can ignore only react from core libs

villesau · 2019-06-08T09:38:30Z

Oh, the perf issue sounds significant :/

In case of TS I think it does not have (almost) any builtin library definitions. Everything tends to live in definitelytyped, even Node definitions: https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/node

Hypnosphi · 2019-09-03T10:49:41Z

Will it fix the following false negative?

https://flow.org/try/#0PQKgBAAgZgNg9gdzCYAoVBLAtgBzgJwBdkwBDAZzACUBTUgY2KnzizAHJ87H31hgwAEgCiADxw1Gw-C3wAuMAAsuUMOUVwArjAAmYAEY01hfBgB2Ac1QAeUkpUBeAN4BGAEwBmAL5hgAPlQgA

/* @flow */

import * as React from 'react'

// $ExpectError: href should be string
<a href={123} />

goodmind · 2019-09-03T11:28:15Z

Yes, it should fix it

facebook-github-bot added the CLA Signed label Mar 18, 2019

villesau force-pushed the stronger-react-dom-2 branch 2 times, most recently from 8a65145 to adb4f74 Compare March 18, 2019 21:42

goodmind added the Library definitions Issues or pull requests about core library definitions label Mar 19, 2019

nmote assigned panagosg7 Mar 26, 2019

facebook-github-bot reviewed Apr 1, 2019

View reviewed changes

panagosg7 reviewed Apr 10, 2019

View reviewed changes

goodmind mentioned this pull request Apr 15, 2019

Define prop types for HTML elements e.g. <input> #7644

Open

motiz88 and others added 8 commits May 7, 2019 23:22

Create placeholder prop types for react-dom HTML/SVG intrinsics

93e4410

React DOM: Add autoCapitalize HTML prop

415976e

React DOM: Restrict content props on void HTML elements

213201f

Add definition for React DOM SyntheticPointerEvent

a402a3e

This is based on facebook/react#12507 which is seemingly missing a couple of fields from the official spec. Consequently this will need an update once React supports all fields.

Add definitions for all React DOM events to HTML JSX intrinsics

aedba0d

This also refactors the main set of JSX intrinsics declarations to support typing Event.currentTarget with each element's instance type.

Remove empty string from React DOM booleans/enums

694e943

This is a design choice documented in facebook#6727 (comment)

update tests

5c2f919

villesau force-pushed the stronger-react-dom-2 branch from adb4f74 to 5c2f919 Compare May 7, 2019 21:00

Ville Saukkonen added 2 commits May 8, 2019 00:11

allow null in class name

028b316

allow string in ReactDOM$Number

ab9d235

facebook-github-bot reviewed May 20, 2019

View reviewed changes

Hypnosphi mentioned this pull request Sep 3, 2019

Fix @reach/router Link base type flow-typed/flow-typed#3546

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stronger react dom refreshed #7569

Stronger react dom refreshed #7569

villesau commented Mar 18, 2019 •

edited

goodmind commented Mar 18, 2019 •

edited

villesau commented Apr 1, 2019 •

edited

facebook-github-bot left a comment

villesau commented Apr 10, 2019

panagosg7 left a comment

panagosg7 Apr 10, 2019

panagosg7 Apr 10, 2019

panagosg7 Apr 10, 2019

goodmind Apr 10, 2019

pascalduez Apr 11, 2019

pascalduez Apr 11, 2019

villesau Apr 11, 2019

goodmind Apr 11, 2019 •

edited

villesau May 7, 2019

villesau May 19, 2019

MichaReiser commented Apr 22, 2019

villesau commented May 7, 2019 •

edited

goodmind commented May 20, 2019

facebook-github-bot left a comment

villesau commented Jun 7, 2019 •

edited

panagosg7 commented Jun 7, 2019

villesau commented Jun 7, 2019 •

edited

panagosg7 commented Jun 7, 2019 •

edited

goodmind commented Jun 8, 2019 •

edited

villesau commented Jun 8, 2019

Hypnosphi commented Sep 3, 2019

goodmind commented Sep 3, 2019

Stronger react dom refreshed #7569

Are you sure you want to change the base?

Stronger react dom refreshed #7569

Conversation

villesau commented Mar 18, 2019 • edited

goodmind commented Mar 18, 2019 • edited

villesau commented Apr 1, 2019 • edited

facebook-github-bot left a comment

Choose a reason for hiding this comment

villesau commented Apr 10, 2019

panagosg7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goodmind Apr 11, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaReiser commented Apr 22, 2019

villesau commented May 7, 2019 • edited

goodmind commented May 20, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

villesau commented Jun 7, 2019 • edited

panagosg7 commented Jun 7, 2019

villesau commented Jun 7, 2019 • edited

panagosg7 commented Jun 7, 2019 • edited

goodmind commented Jun 8, 2019 • edited

villesau commented Jun 8, 2019

Hypnosphi commented Sep 3, 2019

goodmind commented Sep 3, 2019

villesau commented Mar 18, 2019 •

edited

goodmind commented Mar 18, 2019 •

edited

villesau commented Apr 1, 2019 •

edited

goodmind Apr 11, 2019 •

edited

villesau commented May 7, 2019 •

edited

villesau commented Jun 7, 2019 •

edited

villesau commented Jun 7, 2019 •

edited

panagosg7 commented Jun 7, 2019 •

edited

goodmind commented Jun 8, 2019 •

edited