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

Convert AddressPage.js to TSX. #346

Merged
merged 16 commits into from
Jul 29, 2020
Merged

Convert AddressPage.js to TSX. #346

merged 16 commits into from
Jul 29, 2020

Conversation

toolness
Copy link
Contributor

@toolness toolness commented Jul 27, 2020

This converts AddressPage.js to TSX. Crucially, though, most of the address-related state fields are typed to any, which makes some of this incomplete. Those fields are going to take a lot of work to properly type because of how they're being used, and how they interact with some components further down in the component hierarchy (some of which are still untyped). It might ultimately be easier to just refactor the data model to be more comprehensible, rather than attempting to add types as-is.

Note that this introduces an explicit throw for the situation where an address search has no geosearch information; this essentially means that the error signature for #340 will now be different, as it will be an explicit exception rather than Cannot get property 'bbl' of undefined or null.

Comment on lines 14 to 31
type Addr = {
housenumber: string;
streetname: string;
zip: string;
boro: string;
bbl: string;
yearbuilt: number;
unitsres: number;
rsunits2007: number;
rsunits2017: number;
openviolations: number;
totalviolations: number;
evictions: string | null;
ownernames: { title: string; value: string }[];
lastsaledate: string;
lastsaleamount: string;
lastsaleacrisid: string;
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, at least we're getting rid of this duplicate typing.

super(props);

this.state = {
searchAddress: { ...props.match.params }, // maybe this should be
userAddr: {}, // merged together?
hasSearched: false,
geosearch: {},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit queasy about this change but hopefully it should work. We should probably kick the tires a bit just to make sure nothing explodes...

@sraby
Copy link
Member

sraby commented Jul 28, 2020

@toolness this is ready for your eyes again!

@toolness toolness changed the title Convert part of AddressPage.js to TSX. Convert AddressPage.js to TSX. Jul 29, 2020
@toolness toolness merged commit bd2168e into master Jul 29, 2020
@toolness toolness deleted the addresspage-partial-tsx branch July 29, 2020 10:32
@toolness
Copy link
Contributor Author

awesome thanks @sraby !

sraby pushed a commit that referenced this pull request Jul 29, 2020
This fixes #352. The bug was indeed introduced by #346, which essentially split what was previously a single state update into two separate ones: this then caused render() to be called in a state where the <NotRegisteredPage> was mounted and immediately unmounted.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants