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

Refactor/rewrite as middleware #5

Merged
merged 11 commits into from
Oct 14, 2016

Conversation

PAkerstrand
Copy link
Collaborator

@PAkerstrand PAkerstrand commented Oct 12, 2016

This PR re-writes react-router-redial to work as a middleware. This allows it to be easily integrated with other middlewares, such as react-router-scroll.

Notable changes

  • useRedial - New function for creating a React Router middleware. Takes the same options as the earlier RedialContext. Instead of calling <Router render={(props) => <RedialContext .../>}/>, we now use applyRouterMiddleware instead: <Router render={applyRouterMiddleware(useRedial(redialOptions))}/>
  • The redial-map is now based on path from Route rather than the Component. This allows us to have multiple routes handled by the same component but with different props. On the other hand, if we want to retain the properties, this is no longer possible. Perhaps we should have a way to force the redial-props key?

Remaining work

  • Right now the prevProps etc doesn't work any longer. How should we handle this? Move the state down to <RedialContextContainer/> instead?

  • Update the documentation to highlight the new approach to render both on server and client.

  • Create an example mixing useRedial with another router middleware

    render={(props) => (
      <RedialContext {...props} {...redialOptions}>
        <RouterContext {...props} 
            createElement={(Component, routeProps) => (
               <RedialContextContainer routerProps={routeProps}>
                 <Component/>
               </RedialContextContainer>
            )
        />
     </RedialContext>
    )}
  • Add tests for the util functions. Also, is there a way to gracefully fail rather than throwing?

  • ... More?

@@ -129,7 +122,7 @@ export default class RedialContext extends Component {
}

reloadComponent(component) {
this.load(component, this.props, true);
this.load([component], this.props, true);
Copy link
Owner

Choose a reason for hiding this comment

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

I do not think we need to do this here, redial should take care of this for us.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reverted

@dlmr
Copy link
Owner

dlmr commented Oct 13, 2016

Thanks a ton, this looks awesome! 👍

Just a quick thought, what do you think about using camelCase for the new util files.

get-route-path.js => getRoutePath.js
find-route-by-component.js => findRouteByComponent.js
map-keys.js => createMapKeys.js

I will take a closer look at this tomorrow and play around with it a little.

@PAkerstrand
Copy link
Collaborator Author

Oh absolutely, just an old habit I guess. Renaming would make it more consistent!

@dlmr
Copy link
Owner

dlmr commented Oct 14, 2016

This works great, awesomely done once again! 👍

I will go ahead and merge this and release it as 0.3.0 soon!

I will add you as contributor in the README.md and package.json if you don't mind along with giving you write access to the repo.

@dlmr dlmr merged commit 5c71d28 into dlmr:master Oct 14, 2016
@PAkerstrand PAkerstrand deleted the refactor/rewrite-as-middleware branch October 14, 2016 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants