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

isolate proposal: a different scope for each driver #526

Closed
abaco opened this issue Feb 22, 2017 · 53 comments
Closed

isolate proposal: a different scope for each driver #526

abaco opened this issue Feb 22, 2017 · 53 comments

Comments

@abaco
Copy link
Contributor

abaco commented Feb 22, 2017

How about letting isolate accept multiple scopes? So instead of:

isolate(Child, 'child')({onion, HTTP})

one could also do:

isolate(Child, {onion: 'child', HTTP: 'xyz'})({onion, HTTP})

This would solve for instance staltz/cycle-onionify#8:

isolate(Child, {onion: 'child', HTTP: 'xyz'})({onion, HTTP})
isolate(AnotherChild, {onion: 'child', HTTP: 'uvw'})({onion, HTTP}) 

When the scope for a driver is omitted, one would be internally generated, as usual:

isolate(Child, {onion: 'child'})({onion, HTTP})
isolate(AnotherChild, {onion: 'child'})({onion, HTTP}) 

I've already implemented this in a branch.

In addition to this change I also removed the conversion of scopes into strings (e.g. when you provide a number) because I need that for an other experiment with onionify. This can break isolation in some drivers, e.g. in the DOM driver when you pass a number as scope.

@staltz staltz changed the title @cycle/isolate proposal: each driver/wrapper can get a different scope isolate proposal: a different scope for each driver Feb 23, 2017
@staltz
Copy link
Member

staltz commented Feb 23, 2017

Very interesting proposal. I like how isolate(Child, {onion: 'child', HTTP: 'xyz'})({onion, HTTP}) looks rather clean. It would also not be a breaking change.

In addition to this change I also removed the conversion of scopes into strings (e.g. when you provide a number) because I need that for an other experiment with onionify. This can break isolation in some drivers, e.g. in the DOM driver when you pass a number as scope.

So, unrelated to this proposal, right?


Any feedback @Widdershin @TylorS?

@abaco
Copy link
Contributor Author

abaco commented Feb 23, 2017

So, unrelated to this proposal, right?

Yes, only loosely related. I just mentioned it because:

  1. I've already implemented the change, but beware of my code because it's also doing something else;
  2. IMO the ability to pass non-string scopes to isolate would give more value to the present proposal.

So I would first discuss that "non-string scope proposal" in staltz/cycle-onionify#11, where it's used for a clear, practical purpose. Let's use this issue only for the non-breaking change.

@aronallen
Copy link
Contributor

I like this.

I haven't run into this issue yet, but is it possible for isolate only to isolate certain drivers?.
Passing a null value as a scope identifier could be a way to disable isolate for that specific driver.

isolate(Child, {onion : 'child', HTTP: null})({onion, HTTP})

@aronallen
Copy link
Contributor

Another option could be to pass a second set of drivers that bypass isolate.

isolate(Child, {onion : 'child'})({onion}, {HTTP})

@abaco
Copy link
Contributor Author

abaco commented Feb 24, 2017

@aronallen Very good point.

Your suggestion of using null to mean "no isolation" looks quite intuitive, but I wonder whether there could be a better option. In onionify as modified for staltz/cycle-onionify#11 there is already a way of avoiding isolation, namely passing the identity lens as scope. Isn't it possible to have something like that in the HTTP driver, too? What if the driver exported a type enum HTTPIsolateScope {Do, Dont}, and only accepted scopes of that type? Wouldn't those two values be sufficient for the driver to understand how to isolate its sources and sinks?

(Thinking of #441, the DOM driver could accept scopes of type enum DOMIsolateScope {Dont, ParentChild, Siblings, ParentChildSiblings}).

I think it's more consistent for isolate to just pass the scopes it gets to the appropriate drivers, without interpreting them. We could support

isolate(Child, {onion : 'child', HTTP: null})({onion, HTTP})

but let the HTTP driver, not isolate, understand that null means "don't isolate". The API would be the same, but all drivers would have to be modified.

@davidskuza
Copy link

Do we use somewhere nulls already? Do we want "A functional and reactive JavaScript framework" to force users into nulls?

@staltz
Copy link
Member

staltz commented Feb 24, 2017

Good point. Currently undefined scope means "random generated scope". Adding null with "no scope" semantics could be confusing. Not to mention the reliance on null.

That said, I still like Aron's idea for a simple way of choosing no isolation for some sources/sinks.

@abaco
Copy link
Contributor Author

abaco commented Feb 24, 2017

What about the empty string?

@staltz
Copy link
Member

staltz commented Feb 24, 2017

😬

@staltz
Copy link
Member

staltz commented Feb 24, 2017

let the HTTP driver, not isolate, understand that null means "don't isolate". The API would be the same, but all drivers would have to be modified.

I like this idea.

@abaco
Copy link
Contributor Author

abaco commented Feb 24, 2017

I think we're hitting a wall. Strings contain a lot of information in terms of bits, but not the kind of information drivers need to do their isolation stuff. We can use null for the sake of backwards compatibility, but that's not good IMHO.

@staltz
Copy link
Member

staltz commented Feb 24, 2017

Here's a different way of thinking about this whole issue: should we allow each channel [1] to define its own isolation instructions? For instance, the onion channel can take lenses or strings as scopes, and the identity lens means no isolation. For other channels like the DOM, we could have an enum or something else. I thought even about the string ':root' for the DOM channel to mean "use the same isolation scope as the parent", in essence, no isolation.


[1] we've been calling it "drivers", but what we mean is simply the property foo in sources.foo and sinks.foo. I'll call foo a "channel" instead. I find it very suiting because by the dictionary it means "the bed of a stream or river", which is exactly what we want. Not every foo is related to a driver, like onion isn't.

@staltz
Copy link
Member

staltz commented Feb 24, 2017

And by the way, I don't think the enum Dont, ParentChild, Siblings, ParentChildSiblings would work for the DOM channel because then by the laws of referential transparency in FP,

const First = isolate(Child, {DOM: ParentChild})
const Second = isolate(Child, {DOM: ParentChild})

First and Second should be exactly the same thing because the arguments provided were the same. I don't think we can escape names (or can we?). Also, the enum is complicated because true enums only exist in TypeScript, as well as not being able to use the same scope for all channels if someone wants to.

Also because of backwards compatibility, we are probably still going to primarily use strings as scopes. Because this is JavaScript, we shouldn't try to fight the language so much. I admit in a different language we'd be using ADTs for this.

@abaco
Copy link
Contributor Author

abaco commented Feb 24, 2017

Doesn't

const First = isolate(Child)
const Second = isolate(Child)

have the same referential transparency issue as

const First = isolate(Child, {DOM: ParentChild})
const Second = isolate(Child, {DOM: ParentChild})

?

I'm not sure we can escape names, but my point with enums was that maybe we can. Maybe scopes should convey just enough information for the channel to understand how to isolate. I'd like to really understand whether our use of strings is just a leak of implementation details, a necessity of JavaScript, a legacy, or whatever else. Anyways I'm totally fine in using strings if that's the best compromise.

@aronallen
Copy link
Contributor

Instead of using strings as scope identifiers, we could use symbols. Then isolate could export a special symbol that means no isolation. This way we avoid giving meaning to null.

@aronallen
Copy link
Contributor

We could also just reserve a string, Like "!" or "*" but i prefer the symbol solution.

@abaco
Copy link
Contributor Author

abaco commented Feb 24, 2017

@aronallen Symbols are cool.

Anyway, @staltz, I do see your point with referential transparency. At least now we have a way to write orthodox FP code if we want, while with my proposal it would be impossible.

What about the DOM driver exporting a function parentChild(scopeName?: string) that generates a parent-child isolation instruction? So we could do:

const First = isolate(Child, {DOM: parentChild('first')})
const Second = isolate(Child, {DOM: parentChild('second')})

And maybe also a defaultScope(scopeName: string) => parentChild(scopeName) that would be used in cases like

const First = isolate(Child, 'first')
const Second = isolate(Child)

@mightyiam
Copy link

Symbols are good for isolation because (except for Symbol.for), they can be obtained only where they were created. So they must be passed or exported somehow. Good for isolation.

@mightyiam
Copy link

That is, unlike a string, they can't simply appear again like the same string can be written in two different scopes independently.

But the actual mechanism of isolation in the DOM relies on classes. And those are strings...

@abaco
Copy link
Contributor Author

abaco commented Feb 24, 2017

Unfortunately symbols are the most referential opaque thing on earth... :( I think we do care about referential transparency after all.

@mightyiam
Copy link

mightyiam commented Feb 24, 2017 via email

@abaco
Copy link
Contributor Author

abaco commented Feb 24, 2017

That's true. Silly me.

@staltz
Copy link
Member

staltz commented Feb 24, 2017

Just to make sure we are all on the same page, I'll describe how isolation works today:

Channel isolateSource isolateSink Meaning
DOM with scope='foo' domSource .select('$$CYCLEDOM$$-foo') Set vnode.data.isolate=foo Child-parent isolation + sibling isolation
HTTP with scope='foo' httpSource.filter('foo') Append 'foo' to the request's namespace Sibling isolation
Onion with scope='foo' state$.map(state => state.foo) Return an outer reducer that applies the inner reducer to outerState.foo Sibling isolation

Some notes:

We don't have "Channel with scope=undefined" because that's never the case. isolate will create a random scope string if we provided undefined, so by the time we call isolateSource/isolateSink, we are always providing a string.

Then, notice that in every case, isolateSource works as "zoom-in" while isolateSink means "zoom-out". For instance, check how onionify's isolateSource gets an outer (parent) state and returns an inner (child) state, while isolateSink takes an inner reducer and returns an outer reducer.

Also notice that in the DOM channel, .select('$$CYCLEDOM$$-foo') works differently than .select('.foo'). The first case will create an "isolation scope" (magic stuff), while the second case will create a selector that works like querySelectorAll. This means the DOM channel already has some kind of distinction between "normal select" and "special select", not so different to onion channel's "normal select" (with strings) and the proposed "special select" with lenses object.


Now, new ways how we could change isolation (I'll list all applicable, I'm not saying which ones of these will we support):

Channel isolateSource isolateSink Meaning
Onion with scope=lensObj state$.map(state => lensObj.get(state)) lensObj .set(outerState, innerState) Custom isolation
HTTP with scope=null identity fn identity fn No isolation
DOM with scope=null identity fn identity fn No isolation
DOM with scope=':root' identity fn identity fn No isolation
DOM with scope='.foo' domSource.select('.foo') Add foo in the className for vnode Sibling isolation
DOM with scope='#foo' domSource.select('#foo') Set foo as the id for vnode Sibling isolation
DOM with scope=sibling('foo') domSource.select('.foo') Add foo in the className for vnode Sibling isolation
DOM with scope=total('foo') domSource .select('$$CYCLEDOM$$-foo') Set vnode.data.isolate=foo Child-parent isolation + sibling isolation

Does this give anyone some insights?

@staltz
Copy link
Member

staltz commented Feb 24, 2017

In my opinion, for the DOM channel I think we can support isolateSource(domSource, '.foo') which will achieve sibling isolation (no parent-child isolation), so this way we don't have to introduce sibling('foo') / total('foo') helper functions, and we can support the currently existing way of having total isolation through isolateSource(domSource, 'foo')

@mightyiam
Copy link

@staltz how do you produce so much awesome in so little time, so frequently? 🙇‍♂️

@abaco
Copy link
Contributor Author

abaco commented Feb 27, 2017

@davidskuza Your proposal would break all existing code, and I don't think it's worth it. I also suspect most people would usually want isolation for all channels, only occasionally skipping it for one or two of them. Then your first syntax would be rarely used. (Of course I don't know how people actually use isolate, I'm just guessing).

However your suggestion convinced me we should support all the following APIs:

isolate(Component) // auto-generated scope for all channels
isolate(Component, 'something') // 'something' for all channels
isolate(Component, {HTTP: 'else'}) // 'else' for HTTP, auto-generated for the rest
isolate(Component, 'something', {HTTP: 'else'}) // 'else' for HTTP, 'something' for the rest

The last syntax would give us a straightforward way of writing referentially transparent code. We have that now (2nd syntax), and I think it's nice. With only the 3rd syntax it would be really cumbersome to enforce referential transparency: very verbose, and would require an update whenever a channel is added.

@davidskuza
Copy link

This is probably the best time to make breaking changes, not so many people using it in production yet.
I think your idea of isolate(Component, 'something') // 'something' for all channels is ok, I'm talking about "do not isolate" case.

Also good time to consider if we want to fix this:

isolate() with an implicit scope parameter is not referentially transparent. In other words, calling isolate() with an implicit scope is “impure”. [...] Since Cycle.js follows functional programming techniques, usually most of its API is referentially transparent. isolate() is an exception

@abaco
Copy link
Contributor Author

abaco commented Feb 27, 2017

The impurity of isolate has been discussed many times. I think it's alright to have the choice to use isolate in an impure way, as long as it's equally easy to use it in a pure way. In other words, I appreciate the fact that the current API is not biased against nor in favour of the impure version. I'd like to keep this feature, hence my proposal isolate(Component, 'something', {HTTP: 'else'}).

Are you proposing isolate(Component, {HTTP: 'something'}) as the main way to bypass isolation? I guess in most cases you want to isolate all channels, and in some cases all except one. Skipping the DOM driver with that syntax would look like this:

isolate(Component, {HTTP: 'something', onionify: 'something', history: 'something'})

while with staltz's proposal:

isolate(Component, {DOM: ':root'})

@davidskuza
Copy link

DOM maybe yes but not every channel will be able to use string. Example on HTTP:

For everything:

isolate(Component, 'something')
// or
isolate(Component, { default: 'something' })

For everything except HTTP:

isolate(Component, { onionify: 'something', history: 'something' })

instead of

isolate(Component, { HTTP: null, onionify: 'something', history: 'something' })

@abaco
Copy link
Contributor Author

abaco commented Feb 27, 2017

What's your answer to staltz's question:

should we allow each channel [1] to define its own isolation instructions?

My answer is yes. Isolation instructions include "no isolation": for onionify that's the identity lens, for the DOM driver it can be the special scope ':root', and for the HTTP driver we'll have to come up with something.

@davidskuza
Copy link

Yes, I agree with you and I want the same. I just proposed this way because we would not have to use null (for "no isolation"). If somebody comes with better idea than that then it's ok for me (even maybe that null is ok, I just try to make "us" to think if there is a better way than null).

@staltz
Copy link
Member

staltz commented Mar 1, 2017

So far the most unknown issue is how to instruct the HTTP driver to do no isolation. Because other than that, I'd be totally in favor of doing the following:

  • isolate to accept multiple scopes in the second argument
  • Scopes can be any type
  • scope=lensObj for onionify
  • scope=':root' as no-isolation for DOM
  • scope='.foo' as sibling isolation for DOM

To be honest, I don't see what's the big problem with null. I understand there could be some confusion around truthy/null/undefined where each one means something different, but most of the times with isolate we aren't passing any scope anyway. So it's undefined but the user isn't explicitly passing undefined. In the long run, we could even get rid of implicit scopes, but let's do it in a way that we deprecate it only when most people aren't using it. If we deprecate it now, it's a hard deprecation and forces people to migrate. I prefer gradual deprecation.

@abaco
Copy link
Contributor Author

abaco commented Mar 1, 2017

I neither have strong objections against null for the DOM driver.

@staltz, what do you think of this syntax:

isolate(Component, 'something', {HTTP: 'else'}) //  'else' for HTTP, 'something' for the rest

@staltz
Copy link
Member

staltz commented Mar 1, 2017

I'm usually against adding more arguments, because they aren't quickly learnable from a glance. For instance in the past we improved from domSource.get('.foo', 'click') to domSource.select('.foo').events('click') because the chained functions tell their purpose. With an API with three arguments, people may forget is it

isolate(C, 'a', {HTTP: 'b'}) or isolate(C, {HTTP:'b'}, 'a')

and may forget or not be able to learn from a glance what is the purpose of the second and the third argument.

I would suggest instead

isolate(Component, {'*': 'something', HTTP: 'else'})

Which makes it a bit better, but still not ideal.

@staltz
Copy link
Member

staltz commented Mar 1, 2017

I neither have strong objections against null for the DOM driver.

I think you meant HTTP?

@abaco
Copy link
Contributor Author

abaco commented Mar 1, 2017

Yes, sorry, I meant HTTP. I like your suggestion isolate(Component, {'*': 'something', HTTP: 'else'})

@aronallen
Copy link
Contributor

aronallen commented Mar 1, 2017

I am leaning towards not defining the value type for each driver, simply letting isolate pass a specific value to a specific driver. It is up to each driver to interpret what undefined or null might mean. This could also allow a driver to use symbols or numbers or object references as "scopes".

As for the '*' would that pass 'something' to any driver that has an undefined scope?

We could also turn everything around, so instead of calling isolate on the Component, we call it on the driver. This would be a breaking change, but I think it makes more sense. since isolate(Component, {HTTP: null})({HTTP}) would effectively be the same as Component({HTTP}).

The syntax would then look like this

Component({DOM: isolate(DOM), HTTP})

It makes it very clear that DOM is isolated, and HTTP is not.

An optional second argument could be the scope, and each driver could have it's own type definition for scopes.

Component({DOM: isolate(DOM, 'scope'), HTTP} })

Instead of calling this new method isolate, we could call it constrain, so there is no confusion.

So instead of "you may isolate a component" we say "you may pass a constrained driver to a component"

Component({DOM: constrain(DOM, 'scope'), HTTP} })

@staltz
Copy link
Member

staltz commented Mar 1, 2017

@aronallen that API wouldn't work because those arguments are sources, but isolate needs to cover both sources (the argument) and sinks (the return).

This is also why I nowadays avoid writing Component({DOM, HTTP}) and instead write Component({DOM: sources.DOM, HTTP: sources.HTTP}) or even

const childSources = {DOM: sources.DOM, HTTP: sources.HTTP}
const childSinks = Component(childSources)

@abaco
Copy link
Contributor Author

abaco commented Mar 1, 2017

@aronallen

I am leaning towards not defining the value type for each driver, simply letting isolate pass a specific value to a specific driver. It is up to each driver to interpret what undefined or null might mean. This could also allow a driver to use symbols or numbers or object references as "scopes".

That's almost what staltz summarized a few posts up. However, for the sake of backwards compatibility:

  • When the second argument to isolate is undefined it will not be passed to channels. A random string scope will be passed instead.

  • When the second argument to isolate is neither undefined nor an object, it will be stringified and passed to all channels. Thus channels must at least accept string scopes - although they can also accept anything else (symbols, numbers, etc.).

@staltz
Copy link
Member

staltz commented Mar 3, 2017

I think we have converged in this issue. Here's the summary of what will be done:

  • Change isolate so the 2nd argument is polymorphic: can either be one scope or an object of scopes-per-channel
  • If the scopes-per-channel object has '*': 'foo', than 'foo' will be used as the default scope for channels not explicitly specified
  • In the scopes-per-channel object, the scope given to a channel will not be stringified
  • Issue Proposal for dealing with shared/derived data staltz/cycle-onionify#11: onionify will accept a lens object as scope
  • DOM channel will interpret the scope ':root' as no isolation
  • DOM channel will interpret the scope '.foo' as isolation-against-siblings
  • HTTP channel will interpret the scope null as no isolation

If anyone has an important and new comment about the proposal above, please speak up! :)

@wclr
Copy link
Contributor

wclr commented Mar 5, 2017

only one comment: keep it simple

@staltz
Copy link
Member

staltz commented Mar 5, 2017

Is it not simple? How? What would you suggest? How would you solve the new problems identified?

@wclr
Copy link
Contributor

wclr commented Mar 6, 2017

How would you solve the new problems identified?

What is the problems? I see there the problem with onionify isolation? Then I would say that isolation should be removed from onionify completely, and all data modification related stuff should be implemented using data oriented library like ramda or immutablejs using lenses or whatever available, it is really easy and explicit, no need to reinvent the wheel. And simple cycle's isolate model currently implemented shoud stay as it is.

This my suggestion. =)

@wclr
Copy link
Contributor

wclr commented Mar 6, 2017

I'm not sure if sophistication of DOM isolation is needed as well, this is again a step to magical behavior.

@staltz
Copy link
Member

staltz commented Mar 6, 2017

isolation was born out of real needs expressed by multiple users, originally here #167.

And this issue in particular has been requested a couple of times, also related #441.

Dismissing people's problems is not a way of solving problems.

@wclr
Copy link
Contributor

wclr commented Mar 6, 2017

Dismissing people's problems is not a way of solving problems.

That was my two cents, I myself like to overcomplicate stuff and invent problems.

@wclr
Copy link
Contributor

wclr commented Mar 8, 2017

HTTP channel will interpret the scope null as no isolation

Why not false or probably any empty value?

@staltz
Copy link
Member

staltz commented Mar 8, 2017

Why not null? :) (by the way, it's already implemented, a few minutes ago)

@staltz
Copy link
Member

staltz commented Mar 8, 2017

This issue is now done and released, check here https://cycle.js.org/releases.html#flexible-isolation

I'll proceed by making lenses support in onionify, too.

@staltz staltz closed this as completed Mar 8, 2017
@staltz staltz moved this from In progress to Done in March in staltz's pipeline Mar 9, 2017
@staltz staltz removed this from Done recently in staltz's pipeline Apr 28, 2017
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

6 participants