Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 31 additions & 1 deletion src/Expensify.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import React, {Component} from 'react';
import {View} from 'react-native';
import get from 'lodash.get';
Copy link
Contributor

Choose a reason for hiding this comment

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

This was imported somewhere else as lodashGet, maybe we should try to import these using a standard naming convention, for easy grepping

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 want to propose something even crazier...

Let's just ditch underscore already and use lodash for everything.
What does underscore have that lodash doesn't?

https://lodash.com/docs/4.17.15#map
https://lodash.com/docs/4.17.15#chain
https://lodash.com/docs/4.17.15#forEach
https://lodash.com/docs/4.17.15#size
https://lodash.com/docs/4.17.15#find
https://lodash.com/docs/4.17.15#uniqueId

the list goes on and on...

I mean heck it even has _.template (but where we're going we don't need templates)

Copy link
Contributor

Choose a reason for hiding this comment

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

I've definitely thought of that in the past as well, and I keep coming back to this:

  1. There is no problem using underscore
  2. Lodash handles some functionality slightly differently than underscore, and if an engineer isn't aware of that, it can lead to difficult to find bugs (this was more of a historical issue and it's possible the APIs are more aligned today than they have been in the past)
  3. Some underscore functionality we use doesn't exist in lodash (eg. findWhere() indexBy() pluck())
  4. Lodash methods can be imported individually for functionality that underscore doesn't have (but the vice versa is not true)
  5. Lodash is 24kb vs underscore 6kb and includes a lot of stuff that would never be used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, it was worth a shot 😛


// import {Beforeunload} from 'react-beforeunload';
import SignInPage from './page/SignInPage';
Expand All @@ -8,6 +10,8 @@ import * as ActiveClientManager from './lib/ActiveClientManager';
import {verifyAuthToken} from './lib/actions/ActionsSession';
import IONKEYS from './IONKEYS';
import WithIon from './components/WithIon';
import styles from './style/StyleSheet';

import {
Route,
Router,
Expand All @@ -23,6 +27,22 @@ class Expensify extends Component {
super(props);

this.recordCurrentRoute = this.recordCurrentRoute.bind(this);

this.state = {
loading: true,
authenticated: false,
};
}

componentDidMount() {
// We need to delay initializing the main app so we can check for an authToken and
// redirect to the signin page if we do not have one. Otherwise when the app inits
// we will fall through to the homepage and the user will see a brief flash of the main
// app experience.
Ion.get(IONKEYS.SESSION)
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be simplified a little bit to pass back just the authToken:

Ion.get(IONKEYS.SESSION, 'authToken', false)

.then((response) => {
this.setState({loading: false, authenticated: Boolean(get(response, 'authToken', false))});
});
}

/**
Expand All @@ -35,13 +55,23 @@ class Expensify extends Component {
}

render() {
if (this.state.loading) {
return (
<View style={styles.genericView} />
);
}

// We can only have a redirectTo if this is not the initial render so if we have one we'll
// always navigate to it. If we are not authenticated by this point then we'll force navigate to sign in.
const redirectTo = this.state.redirectTo || (!this.state.authenticated && '/signin');

return (

// TODO: Mobile does not support Beforeunload
// <Beforeunload onBeforeunload={ActiveClientManager.removeClient}>
<Router>
{/* If there is ever a property for redirecting, we do the redirect here */}
{this.state && this.state.redirectTo && <Redirect to={this.state.redirectTo} />}
{redirectTo && <Redirect to={redirectTo} />}
<Route path="*" render={this.recordCurrentRoute} />

<Switch>
Expand Down
2 changes: 1 addition & 1 deletion src/lib/Network.js
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ function request(command, data, type = 'post') {
})
.then(redirectToSignIn);
}
return setSuccessfulSignInData(response, command.exitTo);
return setSuccessfulSignInData(response, data.exitTo);
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder how much this tripped me up

})
.then((response) => {
// If Expensify login, it's the users first time signing in and we need to
Expand Down
4 changes: 4 additions & 0 deletions src/style/StyleSheet.js
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,10 @@ const styles = {
padding: 20,
},

genericView: {
backgroundColor: colors.heading,
},

signInPageInner: {
marginLeft: 'auto',
marginRight: 'auto',
Expand Down