Skip to content

react-router Make Route autocomplete its props#36765

Closed
TedDriggs wants to merge 1 commit intoDefinitelyTyped:masterfrom
TedDriggs:patch-1
Closed

react-router Make Route autocomplete its props#36765
TedDriggs wants to merge 1 commit intoDefinitelyTyped:masterfrom
TedDriggs:patch-1

Conversation

@TedDriggs
Copy link

@TedDriggs TedDriggs commented Jul 9, 2019

The T extends RouteProps = RouteProps only provides property autocompletion on the first property entered, after which it forgets what type we're using. Adding & RouteProps to the extends React.Component declaration fixes this issue.

Please fill in this template.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Add or edit tests to reflect the change. (Run with npm test.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If changing an existing definition:

  • Provide a URL to documentation or source code which provides context for the suggested changes: N/A
  • Increase the version number in the header if appropriate.
  • If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }.

@TedDriggs
react-router Make Route autocomplete its props
8b63ee6 
The `T extends RouteProps = RouteProps` only provides property autocompletion on the first property entered, after which it forgets what type we're using. Adding `& RouteProps` to the `extends React.Component` declaration fixes this issue.
@TedDriggs TedDriggs requested a review from johnnyreilly as a code owner July 9, 2019 15:59
@typescript-bot typescript-bot added the Popular package This PR affects a popular package (as counted by NPM download counts). label Jul 9, 2019
@typescript-bot
Copy link
Contributor

typescript-bot commented Jul 9, 2019 •
edited
Loading

@TedDriggs Thank you for submitting this PR!

🔔 @sergey-buturlakin @mrk21 @vasek17 @ngbrown @awendland @KostyaEsmukov @johnnyreilly @LKay @DovydasNavickas @huy-nguyen @Grmiade @DaIgeb @egorshulga @rraina @pret-a-porter @t49tran @8enSmith @wezleytsai @eps1lon @HipsterBrown - 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

typescript-bot commented Jul 9, 2019 •
edited
Loading

@TedDriggs 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!

@typescript-bot
Copy link
Contributor

typescript-bot commented Jul 9, 2019

👋 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?

Comparison details 📊
master #36765 diff
Batch compilation
Memory usage (MiB) 158.4 163.0 +2.9%
Type count 68000 70786 +4.1%
Assignability cache size 60596 60515 -0.1%
Subtype cache size 1028 1390 +35.2%
Identity cache size 111 111 0.0%
Language service
Samples taken 839 843 +0.5%
Identifiers in tests 1879 1879 0.0%
getCompletionsAtPosition
    Mean duration (ms) 896.7 912.9 +1.8%
    Median duration (ms) 870.4 883.2 +1.5%
    Mean CV 8.5% 8.5% -0.2%
    Worst duration (ms) 1174.3 1116.5 -4.9%
    Worst identifier Redirect h3
getQuickInfoAtPosition
    Mean duration (ms) 928.6 903.3 -2.7%
    Median duration (ms) 899.9 863.0 -4.1%
    Mean CV 8.3% 8.6% +3.2%
    Worst duration (ms) 1154.3 1248.4 +8.2%
    Worst identifier component location

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!

@typescript-bot
Copy link
Contributor

typescript-bot commented Jul 15, 2019

@TedDriggs 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.

@TedDriggs
Copy link
Author

TedDriggs commented Jul 15, 2019

@johnnyreilly I'm struggling to figure out how this change caused the redux test to fail; any chance I could ask for your help on understanding how those two relate?

@johnnyreilly
Copy link
Member

johnnyreilly commented Jul 16, 2019

@TedDriggs I'm really sorry but I'm kinda maxed out right now and so can't help I'm afraid. My guess is that there's a test in redux which is affected by your changes; though that seems surprising.

@johnnyreilly
Copy link
Member

johnnyreilly commented Jul 16, 2019

Actually I had a quick look at the error in the logs:

ERROR: 23:50  expect  TypeScript@next compile error: 
Type 'State' does not satisfy the constraint 'ReducersMapObject<any, any>'.
  Index signature is missing in type 'State'.

My guess is you're being bitten by new strictness in the TypeScript compiler. You may want to ask one of the TypeScript team to advise... @andrewbranch any thoughts?

@typescript-bot typescript-bot added the Abandoned This PR had no activity for a long time, and is considered abandoned label Jul 16, 2019
@typescript-bot
Copy link
Contributor

typescript-bot commented Jul 16, 2019

@TedDriggs To keep things tidy, we have to close PRs that aren't mergeable but don't have activity from their author. No worries, though - please open a new PR if you'd like to continue with this change. Thank you!

@andrewbranch
Copy link
Member

andrewbranch commented Jul 16, 2019

I haven’t looked at this super closely nor microsoft/TypeScript#32100 super closely yet, but I wonder if they might be related? In general, I would probably recommend against type hacks that make the editing experience nicer in favor of opening TypeScript bugs when the editing experience isn’t good. (That’s not to say we can never accept a clever workaround that makes the experience better, but hopefully if there’s an experience bug, we can just fix it for everyone.)

If you’re determined to keep working on this, I would recommend you give that PR a try in the process ☝️

@andrewbranch
Copy link
Member

andrewbranch commented Jul 16, 2019

I’m looking at the failed build logs now, though, and while my point still stands, I think the build failures are probably completely unrelated to this, as @johnnyreilly posited.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@johnnyreilly johnnyreilly Awaiting requested review from johnnyreilly

Assignees

No one assigned

Labels

Abandoned This PR had no activity for a long time, and is considered abandoned Popular package This PR affects a popular package (as counted by NPM download counts).

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@TedDriggs @typescript-bot @johnnyreilly @andrewbranch