Selective isolation for pass-through components

[axefrog]

Imagine three components, A, B and C. Component A is the root-level "app" component. B is a delegator of sorts; it exists as a child component doing work involving http requests and some other things that require isolation, but does not generate a vtree; it just passes through a vtree from its own child, component C. Because of the way isolation works, event selectors inside C stop working because they're expecting namespaced elements to exist due to isolation in B. But seeing as B never generated a vtree, and so the scope for B does not exist in the final vtree, the DOM selector fails to match on any specified events because it's including the namespace from B in the selector.

I need a way to selectively isolate a component; i.e. in my case I want it to perform isolation on everything except the DOM source/sink. For the moment I'm considering just copying the isolate source code into my project and modifying it, but this is an edge case that may crop up with isolation for other users as well. I would be happy to submit a pull request if we agreed on what an options argument might look like.

I'd propose an overload of sorts. Currently you have this:

const component = isolate(MyComponent, scope);

... where scope is an optional string. If passing the scope argument as a string could be considered shorthand for { scope }, then it would allow for additional options. We could then have this:

const component = isolate(MyComponent, {
    include: ['http']
});

or:

const component = isolate(MyComponent, {
    exclude: ['DOM', 'router']
});

The ideal approach would be automatically avoiding scoping the selector if we're only passing a child vtree through to the parent, but because selection has to be done in advance before it's clear that a vtree is in fact not being generated by component B, I'm not seeing a way that could be done automatically, hence specifying a source/sink whitelist or blacklist as an argument when calling the isolate function.

[staltz]

I thought carefully about this one. I'm extra cautious about adding new APIs (= more API surface), but I couldn't find a problem with this suggestion. Obviously it can still be achieved manually, by calling isolateSink for each desired source type, but then some boilerplate reduction is necessary, and it would end up looking like this anyway.

So, approved from my side. :+1: If you want to help with a PR, just remember to add tests and documentation, besides the implementation itself. :)

Just to check, though, I'd like to hear @laszlokorte's and @TylorS's thoughts.

[wclr]

probably should take not only string names but also driver object as well as arguments.

const component = isolate(MyComponent, {
    exclude: [DOM, router]
});

[axefrog]

@whitecolor That probably wouldn't work because isolation usually takes place before the sources object is available. It might be possible if performing isolation on the fly, but I'd suggest that doing so would probably be an inefficient approach to isolation.

[staltz]

Yeah, has to be strings, as far as I can see.

[axefrog]

@staltz I've now written the code for this, but will have to find some time in the next few days to write the tests before I submit a pull request.

[wclr]

@axefrog

isolation usually takes place before the sources object is available

why? isolate is called inside component function, are params to this function not available at start?

const main = ({DOM, router}) => {
  const component = isolate(MyComponent, {
    exclude: [router]
  })({DOM, router});
}

why this may not work? Imo just more clean and less error-prone version (there is can be mistakes in string names, and if it is an object, it will throw that ref is not defined)

[axefrog]

I was thinking more in terms of pre-isolation. You isolate a component normally when the component itself is defined, which gives you back a function. That function is something you then call at a later time, when you're ready to instantiate the component. I see that you're doing it inline which, in your case I guess is valid. Is the isolation behaviour that you're suggesting (exclude/include) something you would want to use yourself?

[wclr]

usually (and actually correctly) pre-isolation is done this way, using wrapping function:

const MyComponentIsolated = ({DOM, router}) => 
  isolate(MyComponent, {
    exclude: [router]
  })({DOM, router})

creating isolated component just with isolate not with wrapper can lead to incorrect behaviour.

I think both variants can be handled by isolate, just without string names it is less error-prone as I said.

[TylorS]

I think this would be a very good way to help with the growing complexities of Cycle applications :+1:

I think we should use strings for now as destructuring is required with the other approach, which may not always be desired, and may not always be available etc

[wclr]

Destructing of sources is not required for this simantically, beside this is more explicit and readable:

const MyComponentIsolated = (sources, ...rest) => 
  isolate(MyComponent, {
    exclude: [sources.router]
  })(sources, ...rest)

Is there any code smell using source objects instead their string names or what?

[wclr]

Also such API could be supported:

const MyComponentIsolated = (sources, ...rest) => 
  isolate(MyComponent, ['router'])(sources, ...rest)

would be the same as

const MyComponentIsolated = (sources, ...rest) => 
  isolate(MyComponent, {
    include: ['router']
})(sources, ...rest)

[TylorS]

If using objects directly can work then :+1:

Regardless of whatever way it ends up being implemented I think its a good addition.

[staltz]

const MyComponentIsolated = (sources, ...rest) => 
  isolate(MyComponent, ['router'])(sources, ...rest)

I'd rather not have this API. Too much overloading and starts to be less readable. But I agree with all the rest that was said above.

[axefrog]

This is the code I wrote. Have been too busy to write the tests yet, but will submit a PR when I get a chance.

https://gist.github.com/axefrog/b2f4ff4ae5aeb0b47ddb0bf758615e2d

[wclr]

I'm not sure if this API really needed, if the need is cased only by this problem https://github.com/cyclejs/core/issues/290

And you can do it not without some boilderplate but if this need is rare it is not so much code:

Say you want to exlude DOM and have sources input:

const isolatedChild = 
  isolate(scoped => Child({...scoped, DOM: sources.DOM}))
      ({...sources, DOM: {}})

or if you use destructed sources it looks more simple:

const isolatedChild = 
  isolate(scoped => Child({...scoped, DOM}))({HTTP, storage})

If you want to just include one HTTP in isolation:

const isolatedChild = 
  isolate(({HTTP}) => Child({...sources, HTTP}))({HTTP: sources.HTTP})

Or with destructed:

const isolatedChild = 
  isolate(({HTTP}) => Child({DOM, storage, HTTP}))({HTTP})

And intutivly I feel that "partly isolated" component may be a bad pattern in general.

[ivan-kleshnin]

And intutivly I feel that "partly isolated" component may be a bad pattern in general.

Agreed.

[axefrog]

I don't actually disagree. I'll hold off on this pending further discussion on #290 and #226.

[staltz]

Both #290 and #226 should be fixed by Cycle DOM v10 (release candidate). I recommend trying it out

[axefrog]

Awesome, well that means that this "fix" is no longer relevant, so I'll close this issue.