FormidableLabs / redux-little-router

A tiny router for Redux that lets the URL do the talking.
MIT License
1.04k stars 113 forks source link

Replace {} with any #281

Closed NeoLegends closed 6 years ago

NeoLegends commented 6 years ago

Hey! I've got another bug in the TS typings that's waiting to be fixed. :)

{} does not represent a 'plain object' in TS. This leads to problems using state.router.result.someProperty, because TS asserts that someProperty doesn't exist, when in fact, it does. any is the right type to be used here.

codecov[bot] commented 6 years ago

Codecov Report

Merging #281 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #281   +/-   ##
=======================================
  Coverage   98.61%   98.61%           
=======================================
  Files          28       28           
  Lines         362      362           
=======================================
  Hits          357      357           
  Misses          5        5

Continue to review full report at Codecov.

Legend - Click here to learn more Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data Powered by Codecov. Last update 15886ad...a7483b5. Read the comment docs.

parkerziegler commented 6 years ago

Good catch @NeoLegends! I'd still like to make some sort of contract saying that these need to be objects. Perhaps we could use index signatures, similar to how we do with the Routes interface? Let me know your thoughts on this. Something like:

interface ObjLiteral {
    [key: string]: any;
}

I've never been truly certain on how to deal with objects of unknown shape in TS. any is a great escape hatch if we need it, but would like to use something a bit safer if possible. Thanks!

NeoLegends commented 6 years ago

This would prevent setting these to nully values, sounds good! I'll update the PR.

parkerziegler commented 6 years ago

@NeoLegends this looks good to me, finally had some time to review and all seems to be going smoothly. Please pull latest from master into your branch and then push to this PR and I can merge. cc @ryan-roemer @tptee We can cut a patch release later this week to pull in these updated defs along with #280.

tptee commented 6 years ago

🚒

NeoLegends commented 6 years ago

🀟🏻

davidgruar commented 6 years ago

@parkerziegler @ryan-roemer @tptee Any chance of getting this released, please?

parkerziegler commented 6 years ago

Thanks for pinging us @davidgruar! Meant to cut another release after these got merged but forgot.

@ryan-roemer @tptee I'm happy to cut a new release later today, how do we want to do versioning (major/minor/patch)? IIRC, the diff is just changes to TS defs – I'll need to verify that.

parkerziegler commented 6 years ago

@davidgruar v15.1.1 is published to npm. Let us know if you encounter anything funny!

davidgruar commented 6 years ago

Thank you! It would be worth mentioning #280 in the release notes - it resolves the same issue as #282 but for Typescript (and is actually what I most needed this release for).

parkerziegler commented 6 years ago

Good call out, it's been added!