Skip to content

Conversation

@ntilwalli
Copy link
Collaborator

I encountered an issue where the the defined route stream was not emitting even though I could listen to the history$ and the route should have matched. I fixed it by making a common remember call on the StreamAdapter. I haven't tracked down why this fixed my issue, but I created this PR not necessarily to get it merged but to at a minimum have a discussion about the root cause.

…n each member function independently, update dependency versions for xstream-adapter and xstream
@TylorS
Copy link
Member

TylorS commented Jun 18, 2016

I can see how this would work with xstream, because it will continue to be a MemoryStream, but I have to say I'm very surprised the tests continue to pass with Rx 4 and Rx 5 as 'memory' only occurs where it is explicitly called. I'd love to see some example code where the previous behavior broke something if you could find the time to do so.

@ntilwalli
Copy link
Collaborator Author

ntilwalli commented Jun 18, 2016

I've posted a branch of my routing example repo which demonstrates the issue here: https://github.com/ntilwalli/routingWithState/tree/rememberIssue

You'll see that I explicitly add a listener to the history$. You would expect that clicking on any of the links would cause printout from both the define-stream and the history$ but only the define-stream prints... This is effectively the same issue I had except in my original it was the other way around (history$ printed and define-stream did not...) This issue gets fixed if I call remember in the constructor of RouterSource and not in the member functions.

@TylorS
Copy link
Member

TylorS commented Jun 21, 2016

@ntilwalli Does the issue still persist if the member functions still call remember as well as adding remember to the root history$? I think they would be necessary for all other stream libraries other than xstream.

@ntilwalli
Copy link
Collaborator Author

ntilwalli commented Jun 21, 2016

The issue does still persist if the member functions call remember. Here's the console output

from root: Object {pathname: "/", search: "", hash: "", state: undefined, action: "POP"…}
main.js:12484 from history$...: Object {pathname: "/", search: "", hash: "", state: undefined, action: "POP"…}
main.js:12484 from root: Object {pathname: "/", search: "", hash: "", state: undefined, action: "POP"…}
main.js:12484 from define$...: Object {path: "/", location: Object}
main.js:12487 Object {count: 0}
(index):35 Live reload enabled.
main.js:12484 from root: Object {pathname: "/page1", search: "", hash: "", state: undefined, action: "PUSH"…}
main.js:12484 from define$...: Object {path: "/page1", location: Object}
main.js:12484 from root: Object {pathname: "/page1", search: "", hash: "", state: undefined, action: "PUSH"…}
main.js:12484 from define$...: Object {path: "/page1", location: Object}

After the Live reload enabled is when the page is loaded. You'll see that on page load both the define$ and the history$ (non-root, in main...) fire. After the page is loaded, when I click on the Go to page 1, the root history$ stream fires twice along with two fires of the define$ (note the non-root history$ does not fire). I would expect the root history$ to fire once, the define$ to fire once and the non-root history$ stream to fire once.

When I remove the calls to remember in the member functions and put it directly at the root, it behaves as expected.

@ntilwalli
Copy link
Collaborator Author

ntilwalli commented Jun 21, 2016

I've had trouble constructing a failing test-case, but it should be pretty quick to reproduce. Here are the steps:

  1. Pull the repo (ntilwalli/routerWithState) I posted and switch to the rememberIssue branch
  2. npm install -g live-server watchify
  3. npm install
  4. npm run build
  5. npm run serve
  6. Go to http://localhost:8080, click around and check the console...

@TylorS
Copy link
Member

TylorS commented Jun 21, 2016

So I'm not able to reproduce. It seems to be logging everything I'd expect.

image

@TylorS
Copy link
Member

TylorS commented Jun 21, 2016

The initial values from both, and from router... with each route change, and the debug() with each state change

TylorS added a commit that referenced this pull request Jun 21, 2016
@TylorS TylorS self-assigned this Jun 21, 2016
@TylorS TylorS closed this in #56 Jun 22, 2016
TylorS added a commit that referenced this pull request Jun 22, 2016
* fix(multipleListeners): fix for issue in #51

* fix(RouterSource): add back remember to match$
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.

2 participants