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

[RFR] Upgrade React-router #3744

Merged
merged 17 commits into from
Sep 30, 2019
Merged

[RFR] Upgrade React-router #3744

merged 17 commits into from
Sep 30, 2019

Conversation

djhi
Copy link
Contributor

@djhi djhi commented Sep 27, 2019

No description provided.

@djhi djhi added this to the 3.0.0 milestone Sep 27, 2019
@djhi djhi changed the title [WIP] Upgrade React-router [RFR] Upgrade React-router Sep 27, 2019
Copy link
Member

@fzaninotto fzaninotto left a comment

Choose a reason for hiding this comment

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

Awesome!

I think you missed a few instances of withRouter in the documentation.

@@ -40,42 +40,38 @@ const useLogout = (): Logout => {

const store = useStore();
const dispatch = useDispatch();
const history = useHistory();

const logout = useCallback(
(params = {}, redirectTo = defaultAuthParams.loginUrl) =>
authProvider.logout(params).then(redirectToFromProvider => {
dispatch(clearState());
const currentLocation = store.getState().router.location;
Copy link
Member

Choose a reason for hiding this comment

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

Can't you get rid of this one redux usage?

return redirectToFromProvider;
}),
[authProvider, store, dispatch]
[authProvider, store, history, dispatch]
);

const logoutWithoutProvider = useCallback(
_ => {
const currentLocation = store.getState().router.location;
Copy link
Member

Choose a reason for hiding this comment

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

and here, too.

<Provider store={createStore(() => ({}))}>
<Dummy />
</Provider>
<MemoryRouter>
Copy link
Member

Choose a reason for hiding this comment

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

you can pretty much use the TestContext here

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 cleanup those tests and rewrote them using rtl. One of them was useless

@fzaninotto
Copy link
Member

also, do we still use connected-react-router for anything else than sagas?

@fzaninotto
Copy link
Member

And there is the FIXME in the CloneButton, can we get rid of it?

@djhi
Copy link
Contributor Author

djhi commented Sep 30, 2019

also, do we still use connected-react-router for anything else than sagas?

Only place left is in the saving reducer.

And there is the FIXME in the CloneButton, can we get rid of it?

We might remove the comment, however, I'm not sure we should pass the record in the state. Having it in the URL is a good thing imo: explicit, allow to refresh the page, copy/paste the url, etc.

@fzaninotto
Copy link
Member

Only place left is in the saving reducer.

I think we can get rid of this one, as the useUpdate and useCreate hook pass their own loading state. We'd need to pass it down from Edit to SimpleForm instead of selecting the value in SimpleForm.

As it's a BC break, we'd better do it now.

However, I'm not sure we should pass the record in the state. Having it in the URL is a good thing imo: explicit, allow to refresh the page, copy/paste the url, etc.

I agree. Can you check that using the state is never documented anywhere and remove the FIXME then?

@fzaninotto fzaninotto merged commit f048905 into next Sep 30, 2019
@fzaninotto fzaninotto deleted the react-router branch September 30, 2019 13:06
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.

None yet

2 participants