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

React: typed redux store #7578

Merged
merged 9 commits into from May 23, 2018

Conversation

Projects
None yet
5 participants
@bertwin
Copy link
Contributor

commented May 6, 2018

These changes will make the redux store typed.

  • In the reducers, the state type is infered from the initialState
  • These are combined into a RootState type
  • mapStateToProps uses the RootState to infer its return type StateProps
  • This return type is combined with the 'typeof' dispatchToProps DispatchProps to create Component interfaces
  • Where necessary RouteComponentProps were added.

This way the correct types have to be declared only once, in the initialState.

  • Please make sure the below checklist is followed for Pull Requests.

  • Travis tests are green

  • Tests are added where necessary

  • Documentation is added/updated where necessary

  • Coding Rules & Commit Guidelines as per our CONTRIBUTING.md document are followed

@bertwin bertwin force-pushed the bertwin:react-typed-store branch from 9d9b9a1 to 3275ba4 May 7, 2018

@deepu105
Copy link
Member

left a comment

Great work 👏
I'll assign it to me for testing.
Look at my few comments and also look at the build failure

<%_ } _%>
<%_ } _%>
/* jhipster-needle-add-reducer-import - JHipster will add reducer here */

export default combineReducers({
// tslint:disable-next-line:interface-over-type-literal

This comment has been minimized.

Copy link
@deepu105

deepu105 May 8, 2018

Member

cant this be an interface instaed of type?

<%_ } _%>
match: any;
}
export interface I<%= entityReactName %>Props extends StateProps, DispatchProps, RouteComponentProps<{url: string}> {}

This comment has been minimized.

Copy link
@deepu105

deepu105 May 8, 2018

Member

If it works well for autocompletion and refactoring in IDE/editors then this will be great

<%= entityInstance %>: <%= entityInstance %>.entity
});

const mapDispatchToProps = { getEntity };

type StateProps = ReturnType<typeof mapStateToProps>;

This comment has been minimized.

Copy link
@deepu105

deepu105 May 8, 2018

Member

I didnt know you could do it this way TBH 😉

@@ -16,6 +16,15 @@
See the License for the specific language governing permissions and
limitations under the License.
-%>

// TODO: Fix in react-jhipster
// Reason: getRoles from './user-management.reducer';

This comment has been minimized.

Copy link
@deepu105

deepu105 May 8, 2018

Member

may be we should use a custom type for getRoles?

This comment has been minimized.

Copy link
@bertwin

bertwin May 8, 2018

Author Contributor

yes, that's probably better than making id optional for every ICrudGetAction

This comment has been minimized.

Copy link
@bertwin

bertwin May 8, 2018

Author Contributor

it seems getRoles doen't do much , it fetches ${apiUrl}/authorities but nothing is done with the result

@deepu105 deepu105 self-assigned this May 8, 2018

@jdubois

This comment has been minimized.

Copy link
Member

commented May 16, 2018

As @deepu105 is on holidays, who could do the review @sendilkumarn @wmarques @agaspardcilia @chrisdns ? This looks like a cool feature to have, and as it's spread across the code base it can't stay for too long in a separate branch

@chrisdns

This comment has been minimized.

Copy link
Contributor

commented May 16, 2018

It seems all good, just some minor changes to do like removing the
showModalLogin props from the authentication reducer when using OAuth2, removing the user props from the user-management reducer (it seems to be not used anymore).
There are also some troubles with the IRootState when using OAuth2, but it does not seem to be a big deal to fix

entities: [],
entity: {},
entities: [] as I<%= entityReactName %>[],
entity: new <%= entityReactName %>(),

This comment has been minimized.

Copy link
@bertwin

bertwin May 16, 2018

Author Contributor

I'm not so sure about using classes in redux.

In entity.model.ts.ejs there currently are both an interface and a class. Is there any situation thinkable where a class would be used, except for setting the default value?

Maybe the class could be replaced with

// example from travis react-default
export const createDefault = (): IFieldTestEntity => ({
  booleanTom: false,
  booleanRequiredTom: false
});

this function could then be used in the initialState

This would also discourage creating methods on the entity model, since that doesn't fit well with react.

@deepu105

This comment has been minimized.

Copy link
Member

commented May 16, 2018

@jdubois

This comment has been minimized.

Copy link
Member

commented May 18, 2018

@chrisdns @agaspardcilia can you check @deepu105 comment and validate this? This looks awesome and I'd love to merge this

@jdubois jdubois requested review from agaspardcilia and chrisdns May 18, 2018

@chrisdns

This comment has been minimized.

Copy link
Contributor

commented May 18, 2018

There is no problem with IDEs but the current branch is still inducing some error type and some unused variables should be removed when using OAUth2 like noticed previously.

@bertwin bertwin force-pushed the bertwin:react-typed-store branch from cea2e03 to 7e30ce3 May 22, 2018

@bertwin

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2018

@chrisdns the problems with OAuth2 should be solved now.
@deepu105 I removed the type from getRoles

@jdubois I did a rebase to the current master, because the branch was getting far behind

@deepu105

This comment has been minimized.

Copy link
Member

commented May 22, 2018

@bertwin great I'm gonna test this tomorrow. There is a CI failure, I restarted it, lets see

@deepu105 deepu105 referenced this pull request May 22, 2018

Closed

Entity with string "status" field breaks React reducers #7526

1 of 1 task complete
@jdubois

This comment has been minimized.

Copy link
Member

commented May 22, 2018

@deepu105 the master build should be broken, I reverted something this morning, as I thought you wanted me to do that, but now I'm sure :-(
Sorry I'm at my client office, no time to check or do better - but I'll have time tomorrow at Ippon!!

@deepu105

This comment has been minimized.

Copy link
Member

commented May 22, 2018

@bertwin bertwin force-pushed the bertwin:react-typed-store branch from 386683b to 4fa6c17 May 22, 2018

@bertwin bertwin force-pushed the bertwin:react-typed-store branch from 4fa6c17 to 8011f28 May 22, 2018

@deepu105 deepu105 merged commit 8011f28 into jhipster:master May 23, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
@deepu105

This comment has been minimized.

Copy link
Member

commented May 23, 2018

@bertwin this is really great, much much better than what I had originally. Thank you for this. Expecting a lot more improvements from you 😄

@jdubois

This comment has been minimized.

Copy link
Member

commented May 23, 2018

Awesome! Thanks @bertwin

@bertwin bertwin deleted the bertwin:react-typed-store branch May 23, 2018

@pascalgrimaud

This comment has been minimized.

Copy link
Member

commented on 7dbf05c May 25, 2018

@bertwin @deepu105 : just to let know, it seems to broke the build oauth2+react..
See https://travis-ci.org/hipster-labs/jhipster-travis-build/builds/383058782
Sorry, I can't try to reproduce or to confirm, I'm with my phone

This comment has been minimized.

Copy link
Contributor Author

replied May 25, 2018

Thanks! I will fix the test.

@deepu105 do you think it makes sense to split this reducer?

  • a core that holds the account and isAuthenticated
  • all the login stuff in separate modules for session, oauth2 etc.

I don't know how hard it is to separate things, but i think it would be easier to maintain.

This comment has been minimized.

Copy link
Contributor Author

replied May 25, 2018

This should fix the errors #7689

This comment has been minimized.

Copy link
Member

replied May 26, 2018

@jdubois jdubois added this to the 5.0.0-beta.2 milestone Jun 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.