Skip to content
This repository was archived by the owner on Oct 26, 2018. It is now read-only.

Refactor initialization logic #83

Merged
merged 3 commits into from
Dec 7, 2015
Merged

Refactor initialization logic #83

merged 3 commits into from
Dec 7, 2015

Conversation

jlongster
Copy link
Member

Here's my take on how we should solve some of our initialization issues:

  1. Just change initialState to the current location on the first history.listen call (which is called immediately) (fixes devtools "revert")
  2. Use the lastRoute as the flag to detect if it's the first history.listen call and also set it too (avoids making the store subscriber do another pushState, previously lastRoute === undefined was making it do that). (fixes code ike Don't navigate immediately on boot. #81)
  3. Create an INIT_ACTION so that people can still listen to UPDATE_ACTION just for changes. This is probably the most controversial. What should we do? Is it nice for UPDATE_ACTION to run through the reducers on page load or not? I would think it would not be.

@jlongster
Copy link
Member Author

Note that I'm fine releasing 1.0 as is later tonight unless we magically agree with this and it goes in.

@STRML
Copy link
Contributor

STRML commented Dec 6, 2015

Re: INIT_PATH and UPDATE_PATH, I agree that it much nicer to listen to changes only - that's going to be the most common use of the listener, and without the separation users will have to find a way to distinguish the initial route from updates. It's a breaking change ofc but I believe it's suitable for a 1.0.

This gets a 👍 from me.

if(routing === initialState) {
routing = firstRoute;
}

// Only trigger history update is this is a new change or the
// location has changed.
Copy link
Member

Choose a reason for hiding this comment

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

Tiny typo: "Only trigger history update is" should be "Only trigger history update if"

@ellbee
Copy link
Member

ellbee commented Dec 6, 2015

👍 from me too.

Points 1 and 2 look good. Adding the INIT_ACTION makes sense. I don't think I would expect that UPDATE_ACTION would run on page load.

@kimjoar
Copy link
Collaborator

kimjoar commented Dec 6, 2015

👍 :shipit:

In my testing I found no problems, so I think we should try to get this into 1.0 :)

@jlongster
Copy link
Member Author

Ok cool! I'll go ahead and merge this. We can always make new releases if we find new problems.

FWIW, I tried an approach that got rid of changeId completely. I liked changeId especially for tests, but it sounds like that might be too much of an implementation detail to use for all tests. I tried an approach mentioned in another thread where the state for avoiding the route update is kept just as a local variable:

let avoidRouterUpdate = false;

history.listen(location => {
  // ...

  avoidRouterUpdate = true;
  store.dispatch(pushPath(path, state));
});

store.subscribe(() => {
  // ...

  if(lastRoute !== getRouterState() && !avoidRouterUpdate) {
    history.pushState(...);
  }
});

I realized that we can just check the router state instances to see if they have changed. I got really close, but unfortunately could not get one last test to pass.

This exercise made me really like changeId again, because it's something simple that allows me to easily rationalize about why a router update occurred or not. In my other code I kept having to console.log various variables, and debug why the check wasn't working. With changeId I can just log all actions and generally get the info.

@jlongster
Copy link
Member Author

I'll merge this now and update the docs. Feel free to open new PRs if anything breaks or there are any other improvements you find.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants