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

fix: handle undefined prevroute during createStore/firstRoute #27

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

stevelr
Copy link

@stevelr stevelr commented Apr 7, 2019

Inside createStore, during execution of firstRoute(), req.prevRoute is undefined, so when the middleware chain gets to call("beforeLeave", { prev=true }), route becomes undefined, and it crashes with "no beforeLeave on undefined".
I've been using rudy for over a month without running into this, so there is probably another issue in my code or build process, but this change made it work for me.

@hedgepigdaniel hedgepigdaniel changed the title handle undefined prevroute during createStore/firstRoute fix: handle undefined prevroute during createStore/firstRoute Apr 8, 2019
@hedgepigdaniel
Copy link
Collaborator

Excellent, thanks!

const prevRoute = kind === 'init' ? routes[prev.type] || {} : routes[type]

It looks like prevRoute might be {} or undefined - undefined in the case that kind !== 'init' and the current action type is not in routesMap (e.g. its NOT_FOUND and you haven't defined that). Perhaps before prevRoute was ending up as {}, and that was enough to avoid the crash?

I prefer the idea that if there is no previous route, prevRoute is undefined/null rather than an empty object (since anyone referring to it should be accounting for the concept being potentially invalid, e.g. on the first route, or from a 404). I think we should make that consistent, and add some unit tests to cover

  • The creation of the request so that it returns the correct prevRoute
  • The call middleware so that it responds correctly to different values of prevRoute

If you wanted to work on that there's some unit tests here you can use as an example: 77ad467

Otherwise I'll aim to do it soon.

@@ -21,6 +21,7 @@ export default (name, config = {}) => (api) => {

return (req, next = noOp) => {
const route = prev ? req.prevRoute : req.route
if (!route) return next()
Copy link
Member

Choose a reason for hiding this comment

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

awesome find

@ScriptedAlchemy
Copy link
Member

@hedgepigdaniel lets catch up I think it's time i get some skin in this game with you

@hedgepigdaniel
Copy link
Collaborator

@stevelr Any chance you can create a simple reproduction of this?

I'm struggling to understand how and why route could be undefined in that case.

@hedgepigdaniel
Copy link
Collaborator

Some background:

  • formatRoutes is called in createRouter which should be before any actions are dispatched, and it adds a route for NOT_FOUND in case that's the trigger.
  • Where prevRoute is set in createRequest, const prevRoute = kind === 'init' ? routes[prev.type] || {} : routes[type] even if routes[prev.type] is undefined, prevRoute would still end up as {}.

@ScriptedAlchemy
Copy link
Member

@stevelr bump on reproduction

@stevelr
Copy link
Author

stevelr commented Nov 10, 2021

I'm the last one mentioned here so I hope this wasn't blocked on me. I'm not a rudy user and I don't know if this is even still relevant. I shouldn't be on the decision chain for this.

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.

3 participants