[react-router] Improve withRouter definition with conditional types#24586
[react-router] Improve withRouter definition with conditional types#24586Grmiade wants to merge 3 commits into
Conversation
Grmiade
force-pushed
the
feature/grmiade/react-router-improvements
branch
from
5cf3191 to
524e3c4
Compare
typescript-bot
added
Popular package
|
@Grmiade Thank you for submitting this PR! 🔔 @Shearerbeard @asvetliakov @seryl @dijimsta @me @grossbart @wereHamster @jgoz @tomwanzek @gustavderdrache @borisyankov @Ledragon @Ledragon @dmitryrogozhny @eelco @ghotiphud @schwers @michael-yx-wu @willisplummer @smvilar @MarianPalkus @NoHomey @jwbay @huhuanming @MartynasZilinskas @thovden @KonstantinKai @martynaskadisa @janaagaard75 @ssanchezmarc @fhelwanger @incleaf @pepaar @stephenjelfs @ilivit @mrk21 @Cobster @atd-schubert @JeffJacobson @honzabrecka @antonvasin @steller @dborysov @leonard-thieu @choffmeister @Steve-Fenton @Diullei @tasoili @jasons-novaleaf @seanski @Guuz @ksummerlin @basarat @nwolverson @derekcicerone @AndrewGaspar @seikichi @benjaminjackman @s093294 @JoshStrobl @johnnyreilly @DickvdBrink @King2500 @stkb @DovydasNavickas @bcherny @cyrilletuzi @alejo90 @dobrud @patrickr @ngbrown @theigor @alitaheri @herrmanno @DaIgeb @allienna @schlesingermatthias @InsidersByte @artyomsv @dan-j @minodisk @coding2012 @aaronphy @grahammendick @stevegeek @alexgorbatchev @nupplaphil @mihe @DenisTirilis @mantasmarcinkus @mattoni @paustint @j-fro @bbenezech @pzavolinsky @digiguru @ericanderson @morcerf @tkrotoff @onigoetz @richseviora @theruther4d @ssyrell @endigo @prakarshpandey @forabi @crohlfs @nicolas-schmitt @pjo256 @robessog @tbayne @cdeutsch @rosskevin @varHarrie @bradleyayers @piotrwitek @pikpok @mhegazy @walkerburgin @vsiao @danilojrr @Batbold-Gansukh @octatone @chengsieuly @mretolaza @katbusch @vitosamson @LKay @aaronbeall @ssi-hu-antal-bodnar @flaub @alelode @UJosue10 @dawnmist @Ogglas @Guymestef @radziksh @KostyaEsmukov @mitsuruog @kandros @9renpoto @screendriver @Kimahriman @markspolakovs @mntdn @santiagodoldan @mabels @BernabeFelix @David-LeBlanc-git @kittimiyo @andrewBalekha @smrq @Rogach @royxue @KoalaHuman @uncovertruth @Artur-A @oizie @gustavohenke @mleko @cleverguy25 @matdube @LynxEyes @goblindegook @benbayard @codeaid @jurosh @eugrdn @patsissons @apare @jankarres @deviousm @senukartur @begincalendar @pushplay @timurrustamov @dublicator @vincaslt @gavingregory @cameron-mcateer @brmenchl @invliD @abirkholz @ZheyangSong @andrewhathaway @grzesie2k @evanbb @isman-usoh @lith-light-g @sammkj @yuit @jacekjagiello @tock203 @GiedriusGrabauskas @chnoch @adamwpc @rhysd @Lapanti @psrebniak @bgrieder @cdroulers @gyzerok @tillwolff @bhouser @kristerkari @formatlos @seansfkelley @DanielRosenwasser @bendxn @spielc @gnestor @iamdanfox @sirreal @iplus26 @KurtPreston @m0a @danzel @davschne @buptyyf @tomshen @artfuldev @lavoaster @CarlosBonetti @morphologue @bradzacher @marcfallows @tkqubo @drewnoakes @homburg @ttamminen @hallowatcher @peterblazejewicz @jnetterf @stepancar @dimitarnestorov @alloy @iRoachie @timwangdev @kamal @nelyousfi @jacobbaskin @jnbt @jmfirth @ifiokjr @egorshulga @PaitoAnderson @connectdotz @plantain-00 @mrand01 @CaiHuan @SahinVardar @sivolobov @mhcgrq @kaoDev @abrahambotros @fangpenlin @petejkim @phanalpha @charlesfamu @bang88 @svbutko @levito @robertohuertasm @YourGamesBeOver @DeividasBakanas @sztobar @heatherbooker @salim7 @jemmyw @deevus @wouterhardeman @pegel03 @archy-bold @istefo @mdibyo @shuntksh @zzanol @thasner @kenzierocks @clayne11 @tansongyang @nicholasboll @pdeva @clementdevos @Smiche @kulmajaba @graphcool @voxmatt @npirotte @alechill @xaviergonz @sergey-buturlakin @vasek17 @awendland @huy-nguyen @neuoy @ezintz @8398a7 @Hesquibet @giladgray @iebaker @skirsdeda @vujevits @devrelm @onatm @ninjaferret @tehbi4 @misantronic @remojansen @jeroenvervaeke @ricokahler @DomiR @r3nya @charlesrey @jzoric @curtisw0 @sonnysangha @mctep @horiuchi @mxl @psakalo @Havret @mykter @arvitaly @lochbrunner @guilhermehubner @zry656565 @shssoichiro @janslow @mattvperry @ccancellieri @bsurai @guntherjh @wasd171 @szabolcsx @kraenhansen @rogierschouten @sanyatuning @frodehansen2 @kgtkr @chrisgervang @Barrokgl @mthmulders @rapmue @richbai90 @iskandersierra @mrapogee @mc-petry @Valbrand @viggyfresh @janb87 @corydeppen @aikoven @carsonf @bancek @alsiola @silkyfray @priecint @SashaBayan @kourge @zmaybury @mkornblum @fyrkant @joscha @jessepinho @wapgear @martinduparc @ipoul @nicohartto @snerks - please review this PR in the next few days. Be sure to explicitly select If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead. |
typescript-bot
added
Author is Owner
|
@Grmiade 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! |
Grmiade
force-pushed
the
feature/grmiade/react-router-improvements
branch
from
524e3c4 to
e777c4e
Compare
Grmiade
force-pushed
the
feature/grmiade/react-router-improvements
branch
from
e777c4e to
5219599
Compare
|
@johnnyreilly I have this error |
|
I think that may be the only course of action I'm afraid. It's not great.. |
|
|
|
Ouch |
Grmiade
force-pushed
the
feature/grmiade/react-router-improvements
branch
from
30332c6 to
80b7a64
Compare
Grmiade
force-pushed
the
feature/grmiade/react-router-improvements
branch
from
80b7a64 to
2110ad6
Compare
Grmiade
requested review from
DanielRosenwasser,
Diullei,
basarat,
borisyankov and
mhegazy
as code owners
|
|
|
Because of the fact that this would break literally everyone who hasn't upgraded to 2.8, DefinitelyTyped is only compatible with the prior release, and that's enabled by updating the package I think you mentioned. |
| // Thomas Schulz <https://github.com/King2500> | ||
| // Definitions: https://github.com/DefinitelyTyped/DefinitelyTyped | ||
| // TypeScript Version: 2.3 | ||
| // TypeScript Version: 2.8 |
There was a problem hiding this comment.
jQuery does not have a dependency on React. This version bump should not be necessary.
There was a problem hiding this comment.
react => react-mce => tinymce => jquery 😞
Normally, I updated only packages where a dependency error was raised.
If you have a better solution, I take it 😉
There was a problem hiding this comment.
react => react-mce => tinymce => jquery 😞
This is the actual dependency graph. Neither @types/jquery or @types/tinymce should require the version bump.
Could you double check that jQuery indeed causes the error without the version bump?
It should be the case that only dependents of React need to be updated, not dependencies (@Andy-MS ?).
If it is the case that jQuery requires the version bump, I'd be against this change even when 2.8 becomes allowed on DefinitelyTyped. Increasing requirements soon after a new version becomes allowed seems to cause breaks for a lot of jQuery consumers. It'd be prudent to wait even longer to minimize this.
There was a problem hiding this comment.
Same as the comment above by @leonard-thieu with respect to d3 et al. None of the D3 modules has an external dependency outside the D3 ecosystem, the only exception is GeoJson for a couple of module definitions. There is certainly no dependency on react or react-faux-dom.
As for our current versioning policy, we are migrating the definitions to start using some TS 2.3 features, as the release is ~1yr out and the installed based should be able to adopt readily. TS 2.8 is likely a while out, regardless of how beneficial the new features may be.
There was a problem hiding this comment.
Yes I confirm @leonard-thieu 😕
I understand your remarks.
This dependency system is blocking :/ Conditional types feature is sooo powerful to fix some issues.
tomwanzek
left a comment
tomwanzek
left a comment
There was a problem hiding this comment.
Neither d3 nor any of the d3-xxx module definitions have any dependency on React, so beyond the general point made by @DanielRosenwasser, these definitions do not require a TS 2.8, even if one addressed the React part.
@tomwanzek For d3, the dependency path is react => react-faux-dom => d3 |
|
Thanks for your contribution. This PR has failing CI tests and can not be merged in at the time being. For housekeeping purposes we are closing stale PRs. If you'd still like to merge this code in, please open a new PR that has been merged and rebased with the |
7 participants
Thanks to conditional types, withRouter definition excludes injected props only when props is assignable to RouteComponentProps.
Fix #24070