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

chore(react): Clarify React.Attributes #38748

Merged

Conversation

eps1lon
Copy link
Collaborator

@eps1lon eps1lon commented Oct 1, 2019

Second attempt at #38352 which had to be reverted due to a merge conflict in #38603.

The problem was that the actual Props interface of the component didn't match any properties validated in propTypes. While this may be valid js/ts to write it doesn't make a lot of sense. It looks like this passed previously only accidentally due to React.Attributes usage.

@typescript-bot typescript-bot added this to Check and Merge in Pull Request Status Board Oct 1, 2019
@typescript-bot typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). Awaiting reviewer feedback Author is Owner The author of this PR is a listed owner of the package. labels Oct 1, 2019
@typescript-bot
Copy link
Contributor

typescript-bot commented Oct 1, 2019

@eps1lon Thank you for submitting this PR!

🔔 @johnnyreilly @bbenezech @pzavolinsky @digiguru @ericanderson @DovydasNavickas @theruther4d @guilhermehubner @ferdaber @jrakotoharisoa @pascaloliv @Hotell @franklixuefei @Jessidhia @saranshkataria @lukyth - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

@typescript-bot
Copy link
Contributor

Since you're a listed owner and the build passed, this PR is fast-tracked. A maintainer will merge shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped!

@typescript-bot typescript-bot moved this from Check and Merge to Needs Author Attention in Pull Request Status Board Oct 1, 2019
@typescript-bot
Copy link
Contributor

typescript-bot commented Oct 1, 2019

@eps1lon The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

@eps1lon
Copy link
Collaborator Author

eps1lon commented Oct 1, 2019

The errors originates from react-dnd@7.7.0. This particular error is fixed in later versions but would need a backport (react-dnd/react-dnd#1560).

@sandersn
Copy link
Contributor

sandersn commented Oct 1, 2019

@eps1lon I just merged #38736. I'll look at the other two react-dependent failures next.

@sandersn
Copy link
Contributor

sandersn commented Oct 1, 2019

#38777 should fix the other two failures.

@typescript-bot
Copy link
Contributor

@eps1lon I haven't seen anything from you in a while and this PR currently has problems that prevent it from being merged. The PR will be closed tomorrow if there aren't new commits to fix the issues.

@eps1lon eps1lon force-pushed the test/react/attributes-cleanup branch from 0fb92d9 to e713dd6 Compare October 7, 2019 07:32
@typescript-bot typescript-bot moved this from Needs Author Attention to Check and Merge in Pull Request Status Board Oct 7, 2019
@andrewbranch andrewbranch merged commit 508404c into DefinitelyTyped:master Oct 14, 2019
Pull Request Status Board automation moved this from Check and Merge to Done Oct 14, 2019
@typescript-bot
Copy link
Contributor

I just published @types/react@16.9.6 to npm.

@typescript-bot
Copy link
Contributor

👋 Hi there! I’ve run some quick performance metrics against master and your PR. This is still an experiment, so don’t panic if I say something crazy! I’m still learning how to interpret these metrics.

Let’s review the numbers, shall we?

express-session/v1

Comparison details for express-session/v1 📊
master #38748 diff
Batch compilation
Memory usage (MiB) 68.0 63.4 -6.9%
Type count 9972 9968 0.0%
Assignability cache size 3254 3254 0.0%
Subtype cache size 5 5 0.0%
Identity cache size 1 1 0.0%
Language service
Samples taken 126 126 0.0%
Identifiers in tests 126 126 0.0%
getCompletionsAtPosition
    Mean duration (ms) 342.0 345.9 +1.1%
    Median duration (ms) 339.7 340.5 +0.2%
    Mean CV 13.2% 13.2% -0.3%
    Worst duration (ms) 445.5 442.2 -0.7%
    Worst identifier use setHeader
getQuickInfoAtPosition
    Mean duration (ms) 388.2 384.1 -1.0%
    Median duration (ms) 388.6 381.7 -1.8%
    Mean CV 17.5% 15.5% -11.7%
    Worst duration (ms) 493.3 473.8 -3.9%
    Worst identifier sessions Express

It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place.

react/v16

Comparison details for react/v16 📊
master #38748 diff
Batch compilation
Type count 53266 53448 +0.3%
Assignability cache size 23771 23817 +0.2%
Subtype cache size 3886 3898 +0.3%
Identity cache size 764 767 +0.4%
Language service
Samples taken 8788 2121 -75.9%
Identifiers in tests 2524 2544 +0.8%
getCompletionsAtPosition
    Mean duration (ms) 401.8 377.0 -6.2%
    Median duration (ms) 383.4 368.7 -3.8%
    Mean CV 10.3%
    Worst duration (ms) 853.0 785.5 -7.9%
    Worst identifier TransitionGroup TransitionGroup
getQuickInfoAtPosition
    Mean duration (ms) 414.2 389.8 -5.9%
    Median duration (ms) 401.5 381.0 -5.1%
    Mean CV 11.2%
    Worst duration (ms) 836.9 747.4 -10.7%
    Worst identifier component component
System information
Node version v10.15.3 v10.16.3
CPU count 2 2
CPU speed 2.294 GHz 2.394 GHz
CPU model Intel(R) Xeon(R) CPU E5-2673 v4 @ 2.30GHz Intel(R) Xeon(R) CPU E5-2673 v3 @ 2.40GHz
CPU Architecture x64 x64
Memory 6.8 GiB 6.8 GiB
Platform linux linux
Release 4.15.0-1041-azure 4.15.0-1059-azure

First off, note that the system varied slightly between these two runs, so you’ll have to take these measurements with a grain of salt.

It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place.


If you have any questions or comments about me, you can ping @andrewbranch. Have a nice day!

@eps1lon eps1lon deleted the test/react/attributes-cleanup branch October 15, 2019 05:54
chivesrs pushed a commit to chivesrs/DefinitelyTyped that referenced this pull request Nov 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author is Owner The author of this PR is a listed owner of the package. Popular package This PR affects a popular package (as counted by NPM download counts).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants