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

Component runs twice, isolated child components lose scope/namespace #95

Open
wyqydsyq opened this issue Aug 3, 2016 · 16 comments
Open

Comments

@wyqydsyq
Copy link

wyqydsyq commented Aug 3, 2016

I have an app with a bunch of nested components and have found that when I use cyclic-router, the component that's being loaded (Welcome) gets called twice, causing isolated child components with implicit scopes (e.g. export default sources => isolate(UserProfile)(sources)) lose track of their scope because it seems the scopes are recalculated when the component is reran.

If you git clone https://github.com/wyqydsyq/hapi-cycle, run npm install && npm run launch and then visit http://localhost:1337/ you can see the UI in this case works fine, but if you then swap the part of https://github.com/wyqydsyq/hapi-cycle/blob/master/src/ui/app.js that returns just the component with the commented out lines to make it use cyclic-router instead of calling the component directly, subcomponents will no longer emit/recieve events from/to themselves.

Using explicit scopes seems to remedy this, as each isolated child component would be using the same scope string in between calling the main component, but that's more of a band-aid fix than a solution.

So the working/not working cases for me are:

  • ✅ Router with explicit isolation scope on child components
  • ✅ No router (calling component directly) with implied isolation scope on child components
  • ❎ Router with implied isolation scope on child components
@ntilwalli
Copy link
Collaborator

ntilwalli commented Aug 3, 2016

https://github.com/wyqydsyq/hapi-cycle/blob/master/src/ui/app.js#L10

I haven't tried your code but you def need to multicast your routes$. In xstream that's done with remember(). Your function is probably being called twice because you have two sinks that are creating independent closure chains up through the Welcome call, instead of sharing one component emitted from the route.

See example doing this here:
https://github.com/ntilwalli/routingWithState/blob/master/main.js#L91

@wyqydsyq
Copy link
Author

wyqydsyq commented Aug 4, 2016

Ah right I was just going by the example on the readme which doesn't mention it needing to be multicast (though it makes sense that it should be, someone new to Cycle/xstream like myself wouldn't think of that). So I guess the only actionable thing from this issue is to mention that in the docs.

Sorry I'm not sure what you mean by sinks making independent closure chains, could you please further explain or give an example of this?

Thanks.

@ntilwalli
Copy link
Collaborator

ntilwalli commented Aug 4, 2016

Sure. I was being too wordy. If you understand that it needs to be multicast then you understood my point.

For completeness though, the below code will create an object but it won't start emitting until
addListener is called on it:

const a$ = xs.periodic(100).take(5).map(x => x*2)

When you call addListener, a set of closures/functions are created at that time (you can think of a new closure being associated with each operator, in this case the operators are periodic, take, and map) and they call each other in a chain from source to sink ("source" being periodic and "sink" being the functions in the object given to addListener.

Each call to addListener creates a new closure chain, up to the source unless the chain is interrupted by a multicast, in which case the upstream chain is shared.

const a$ = xs.periodic(100).take(5).map(x => x*2).debug()

// Will print 0 0 2 2 4 4 6 6 8 8 10 blue 10 green because each stream below creates an
// independent closure chain
a$.addListener({next: x => {}, error: e => {}, () => console.log(`blue`)})
a$.addListener({next: x => {}, error: e => {}, () => console.log(`green`)})

const b$ = xs.periodic(100).take(5).map(x => x*2).debug().remember()

// Will print 0 2 4 6 8 10 blue green because the streams below share one
// closure chain up to the remember
b$.addListener({next: x => {}, error: e => {}, () => console.log(`blue`)})
b$.addListener({next: x => {}, error: e => {}, () => console.log(`green`)})

@ntilwalli
Copy link
Collaborator

ntilwalli commented Aug 4, 2016

Actually, I just remembered xstream is multicast by default. remember is used to replay the the last value for late listeners. xstream is a newer library so what you experienced may actually be a bug. I'm pretty sure it's not a cyclic-router issue. If remember fixes the issue then it seems to imply that under the hood remember forces an explicit multicast... dunno.

@wyqydsyq
Copy link
Author

wyqydsyq commented Aug 5, 2016

Using .remember() seemed to fix it on Chrome Version 51.0.2704.103 (64-bit) and the latest Safari Tech Preview, but it made no difference on a slightly outdated Chrome (~v49) or on Safari Version 9.1.2 (11601.7.7) which is strange because I'm using the babel polyfill + es2015 preset so assumed it should work the same. Maybe V8 and Apple's new js engine have changed their event loop implementations, affecting streams or something?

Either way it seems this is indeed an xstream bug, but I've only been able to reproduce it when using cyclic-router so far.

@TylorS
Copy link
Member

TylorS commented Aug 5, 2016

Hi guys, thanks for the conversation here! I've been very busy and haven't been able to give this repo much love. PRs are graciously welcomed. Let me know if you would like to be added as a collaborator, I can also probably add you to npm to do new publishes 👍

@wyqydsyq
Copy link
Author

wyqydsyq commented Aug 8, 2016

So I put the app I'm having this issue with up on Heroku and have found something pretty strange - using .remember() on my router stream did fix it in newer browsers locally, but for some reason when served from Heroku the issue resurfaces in the same browsers it works locally in, you can see it in action here:
https://hapi-cycle.herokuapp.com/

If you look at the console when initially loading the page (or clicking one of the buttons that changes the current route) you'll see it's logging component names, and doing it multiple times. These are just from a single console.log() at the top of each component, so you can see it's executing them multiple times.

But when running this exact same app locally, with the same webpack config, the issue doesn't happen and the output is logged once per component.

I'm still trying to make sure this isn't just an error on my part, and pin down where this is happening so I can write a reproducible test/example of it. But it really boggles me as to why it's working for me on localhost in latest browsers, but not at all in older browsers or when served from Heroku. I assumed something in my webpack config/build was breaking it, but even with doing a webpack build with export ENV=production and serving that bundle locally it behaved exactly as my development build (that is, works locally on new browsers, breaks when hosted or using slightly older browsers).

@ntilwalli
Copy link
Collaborator

@wyqydsyq I dunno. I love the goals of xstream, and I've used it a lot, but it is young and def has some bugs. If you have the patience to endure and investigate this issue among others, cool. Otherwise I recommend you switch to another stream library and see if that fixes it.

@TylorS
Copy link
Member

TylorS commented Aug 8, 2016

Apologies for slow response here, and thank you for the discussion. I currently do not have the time to investigate this. I would love for some help and a community contribution.

@TylorS
Copy link
Member

TylorS commented Aug 8, 2016

@wyqydsyq As a potential workaround you could ensure that isolate is only called once, but this may have other issues if you try to use the component multiple times as sibilings to one another.

- export default sources => isolate(Component)(sources)
+ const isolatedComponent = isolate(Component)
+ export default sources => isolatedComponent(sources)

@ntilwalli
Copy link
Collaborator

@TylorS I'd love to be a collaborator, but I don't have anything to contribute yet! Cyclic-router does what I need for now. If I need a feature it doesn't have I'll def ping you abt it and collaborate for sure. :) I find this lib very useful.

@wyqydsyq
Copy link
Author

wyqydsyq commented Aug 9, 2016

@ntilwalli yeah I might switch to most for the time being since it's API is closer to xstream than Rx (plus I wasn't a fan of using Rx back when it was the default for Cycle) but will still try to help track this bug down.

@TylorS thanks for the suggestion, that did help with single isolated components (e.g. the add user form) but I'm also using https://github.com/cyclejs/collection (for listing users) which I can't really apply this work-around to since it seems that library automatically isolates child components behind the scenes.

@wyqydsyq
Copy link
Author

wyqydsyq commented Aug 9, 2016

One thing I just noticed is that it doesn't seem to be just re-calling the routed component once, it calls the component once per each sink I'm mapping over from the router, so for example when I added a Router sink to my main() (previously I was just reading from the Router sources and not passing any router sinks back up) that maps over the Router sink of the routed component, the debug message started firing 3 times instead of 2.

Edit: I just made a test driver to check this and it started logging to console 4 times, so it's definitely calling the routed component once per sink wyqydsyq/hapi-cycle@efc95d1

@wyqydsyq
Copy link
Author

wyqydsyq commented Aug 9, 2016

Alright, now this is some super bizarre shit. Adding .debug('pageChange') to my pages$ stream miraculously fixes this issue. (Which still seems to only occur when using an older browser or serving via Heroku)

This commit exhibits the issue:
wyqydsyq/hapi-cycle@0886ad8

This doesn't:
wyqydsyq/hapi-cycle@d70b9d1

This really confuses me because according to the xstream docs, .debug() should have no effect on the output stream, it's sole purpose is to cause side effects (e.g. console.log()) for debugging from the spy function? Why would adding .debug() change how the stream behaves?

@ntilwalli
Copy link
Collaborator

There's an issue associated with debug oddly causing thing to work: staltz/xstream#91

@ernest-okot
Copy link

Am not sure if this is related, kind apologies in advance if it isn't. I came across this after applying captureClicks to this driver. I realised the logo image on my header was being reloaded every time a route change. Obviously I did not enjoy this very much, after all I had just implemented captureClicks because the browser was loading the whole page every time a link was clicked

Any way here is a watered down version to reproduce

{
  "dependencies": {
    "@cycle/dom": "^17.1.0",
    "@cycle/history": "^6.1.0",
    "@cycle/isolate": "^2.1.0",
    "@cycle/run": "^3.1.0",
    "cyclic-router": "^4.0.3",
    "history": "^4.6.1",
    "object-assign": "^4.1.1",
    "switch-path": "^1.2.0",
    "xstream": "^10.6.0"
  },
}
import xs from "xstream";
import assign from "object-assign";
import {run} from "@cycle/run";
import {makeDOMDriver, div, a, span} from "@cycle/dom";
import {makeHistoryDriver, captureClicks} from "@cycle/history";
import {RouterSource} from "cyclic-router/lib/RouterSource";
import {createBrowserHistory} from "history";
import switchPath from "switch-path";

const makeRouterDriver = (history, routerMatcher) => {
  const historyDriver = captureClicks(makeHistoryDriver(history))

  return sink$ => {
    const history$ = historyDriver(sink$).remember()
    return new RouterSource(history$, [], history.createHref, routerMatcher)
  }
}

const makeRouter = routes => {

  return sources => {

    const match$ = sources.router.define(routes)

    const page$ = match$
      .map(({path, value}) => {
        return value(assign({}, sources, {router: sources.router.path(path)}))
      })

    return {
      DOM: page$.map(c => (c.DOM || xs.never())).flatten(),
      HTTP: page$.map(c => (c.HTTP || xs.never())).flatten(),
      router: page$.map(c => (c.router || xs.never())).flatten(),
      storage: page$.map(c => (c.storage || xs.never())).flatten(),
    };

  }
};

const makeRandom = () => Math.round(Math.random() * 100)

const Main = makeRouter({
  '/home': sources => {

    const id = makeRandom();

    const detail = HomeRouter(sources)

    return {
      DOM: detail.DOM.map(detail => div([
        div(['Home ' + id]),

        div([
          a({attrs: {href: '/home/dashboard'}}, 'Dashboard'),
          span(' - '),
          a({attrs: {href: '/home/settings'}}, 'Settings'),
        ]),

        detail
      ]))
    }
  },
  '*': sources => {
    return {
      DOM: xs.of(div([

        'Not Found',

        div([
          a({attrs: {href: '/home/dashboard'}}, 'Dashboard'),
          span(' - '),
          a({attrs: {href: '/home/settings'}}, 'Settings'),
        ]),

      ]))
    }
  }
})

const HomeRouter = makeRouter({
  '/dashboard': sources => {

    const id = makeRandom();

    return {
      DOM: xs.of(div('Dashboard ' + id))
    }
  },
  '/settings': sources => {

    const id = makeRandom();

    return {
      DOM: xs.of(div('Settings ' + id))
    }
  },
})

const drivers = {
  DOM: makeDOMDriver('#app'),
  router: makeRouterDriver(createBrowserHistory(), switchPath),
}

run(Main, drivers)

Notice how the id value in the main changes every time you click the links. The links open nested components, shouldn't the main component run only once (thus maintain the same value) ?

Has anyone discovered a way around this?

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

No branches or pull requests

4 participants