intercellular / cell

A self-driving web app framework
https://www.celljs.org
MIT License
1.51k stars 94 forks source link

Virus: Plug-in system #173

Closed norchow closed 6 years ago

norchow commented 6 years ago

This is a plug-in system that allows the developer to include his own transformations to the cells, extending them as far as he wants.

To use it, you should "infect" a Gene with a Virus, which is a function that takes a Gene-like object and returns another Gene-like object. This returned object could be a real Gene or an object to be transformed by another Virus (pretty much like a Pipeline)

You can see some examples in the tests included. Both the update propagation and the css-like expansion are real requirements in our system.

Please, let us know any comments and opinions. Any improvement or change request is welcome.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+2.2%) to 86.047% when pulling c5cdc39c050e4fd1d8f213dfd158aaca5b08a15f on bitex-la:virus into cdb2fa87626e284949603eb237f5b213843f9c2f on intercellular:develop.

gliechtenstein commented 6 years ago

This is amazing, thanks for the PR. I've been playing around with it to see what it's capable of as well as looking for any edge cases, here's an example: https://jsfiddle.net/57ww6u70/18/

By the way I noticed an exception handling for when $type is not defined: https://github.com/intercellular/cell/pull/173/files#diff-a0a0700551e574d6bcab870ce2730963R130

This is actually supposed to work on a Phenotype level without worrying about it on the Genotype side (through https://github.com/intercellular/cell/blob/develop/cell.js#L255), which means something like the following should render into a div even when you don't specify the type.

var App = {
  $cell: true,
  $text: "Hello World"
}

Could you try it without that exception handling line and see if it still works?

P.S. Also, I was thinking since this is a huge pillar for the framework, we should create a dedicated root level documentation for it. Was thinking maybe VIRUS.md, and document all the exciting use cases there, including the recursive $update example, which is huge. Would you like to take a stab?

gliechtenstein commented 6 years ago

Added another example tickable to the previous example: https://jsfiddle.net/57ww6u70/22/

Just trying to figure out what the "best practice" is for using this cool new feature :)

norchow commented 6 years ago

First of all, thanks for taking a look into it. We're glad you are interested!

The $type exception handling was to fix this situation: We created a textarea (any non-div element would work for this example) and then, through a virus, we tried to wrap it in a div without setting the $type explicitly. What happened is that the Membrane.build method created the HTML element and set the node as a textarea, so when we replaced it with an implicit div, it wasn't changing. That's why we needed to add a validation to set always explicitly the $type of the resulting gene. We couldn't reproduce this issue in a test, because the jsdom-global does not create real html elements (AFAIK).

Regarding the documentation, i can do it, for sure. As soon as i have some time, i'll write it and share with you in this PR.

Lastly, we were also experimenting some possibilities to find the good practice. We liked the version in which the Virus can accept parameters, but instead of inserting those parameters as attributes in the gene, you can send it directly when setting the virus, like a partial application where the gene is the last parameter. Your last example could be written like this:

var Tickable = function(timer, trigger){
  return function(gene) {
    gene.$init = function() {
      var self = this;
      setInterval(function() {
        self[trigger]();
      }, timer);
    }
    return gene;
  }
}

...

  $virus: [Mutable, Clickable, Tickable(500, '_mutate'), Rectangle]

You can check it out here: https://jsfiddle.net/57ww6u70/26/

gliechtenstein commented 6 years ago

We created a textarea (any non-div element would work for this example) and then, through a virus, we tried to wrap it in a div without setting the $type explicitly. What happened is that the Membrane.build method created the HTML element and set the node as a textarea, so when we replaced it with an implicit div, it wasn't changing.

Could you share a quick example? I'm sorry I don't think I understood the situation. It kind of sounds like a bug in the core library if I understood correctly, but then again, I don't think I understood. I would like to fix it if it's the library's bug that's causing this.


As for the new approach of passing arguments to the virus functions, I've been playing around with it, and I came up with two concerns:

  1. I remember when I first learned jQuery, even though writing a jQuery plugin was easy once you understood the concept, there definitely was a "mental hurdle" getting there, because as a JavaScript newbie some of the concepts were foreign. So writing a jQuery plugin felt like some advanced feat even though it wasn't. I feel like "function as a return value" may be one of those mental hurdles. I liked the simplicity of your original solution where all you need to know is that the gene object gets passed to the virus function and you can do whatever inside the function and just return the mutated gene in the end.
  2. This one's subjective, but I personally liked how the root gene completely describes the behavior in a literal manner (without having to look into all the parameters being passed to the virus functions). One problem I see with passing arguments is that it's not as clear what the end result will be just by looking at the cell object. For example, you can't guess what Tickable(500, '_mutate') will do unless you look into the Tickable function code.

What do you think?

norchow commented 6 years ago

Here is what the example looked like:

function formgroupism(component){
  //This is expected to return a div
  return {
    class: 'form-group',
    $components: [component]
  }
}

var c = {
  $tag: 'textarea#textid',
  $virus: [
    hamlism, //this creates a <textarea id="textid"> (which is correct)
    formgroupism
  ]
}

/* Result:
<textarea class="form-group">
  <textarea id="textid"></textarea>
</textarea>
*/

As i said earlier, it is only reproducible on a real browser.

Regarding the best practices, i agree that is not the easiest mindset to have functions that return another functions, maybe it's only useful for those with a certain background on Functional Programming. However, i think that the beauty of this approach is that you can do both. If you prefer, i can remove the example from the VIRUS.md.

gliechtenstein commented 6 years ago

Regarding the best practices, i agree that is not the easiest mindset to have functions that return another functions, maybe it's only useful for those with a certain background on Functional Programming. However, i think that the beauty of this approach is that you can do both. If you prefer, i can remove the example from the VIRUS.md.

Ah I see, this was my misunderstanding. I got confused thinking we always have to return functions. I agree that the beauty of this is the flexibility and you can do both. Now that I understand, I think it's fine to leave it in the docs. Thanks for the clarification.


As for the example, I am still confused because I tried out the example and I couldn't replicate it in a browser. Maybe I am not doing it right, sorry about this. Here's what I did:

First, I commented out this line: https://github.com/intercellular/cell/pull/173/files#diff-a0a0700551e574d6bcab870ce2730963R129

Then I wrote the following HTML by copy and pasting the test code, and opened in a browser:

<html>
<script src="../cell.js"></script>
<script>
function expand_selector(component, selector){
  let parts = selector.match(/([a-zA-Z0-9]*)([#a-zA-Z0-9-_]*)([.a-zA-Z0-9-_]*)/)
  if (parts[1]) component.$type = parts[1]
  if (parts[2]) component.id = parts[2].substring(1)
  if (parts[3]) component['class'] = parts[3].split('.').join(' ').trim()
  return component
}

function hamlism(component){
  if(component.$components){
    component.$components = component.$components.map(hamlism)
  }

  let tag = component.$tag
  if(!tag) return component

  selectors = tag.split(' ')
  expand_selector(component, selectors.pop())

  return selectors.reduceRight(function(child, selector){
    return expand_selector({$components: [child]}, selector)
  }, component)
}

function formgroupism(component){
  //This is expected to return a div
  return {
    class: 'form-group',
    $components: [component]
  }
}

var c = {
  $cell: true,
  $tag: 'textarea#textid',
  $virus: [
    hamlism, //this creates a <textarea id="textid"> (which is correct)
    formgroupism
  ]
}
</script>
</html>

The result was:

Firefox:

screen shot 2018-03-21 at 4 45 28 am

Chrome:

screen shot 2018-03-21 at 4 46 06 am

Did I miss something? Better yet, could you share a fiddle so I can take a look and see if this is something that needs debugging?

gliechtenstein commented 6 years ago

BTW I can merge if everything is ready to merge, since I already know the code is functional. Please let me know when.

This particular issue I was asking about I can look into later since that line itself isn't that important (Just wasn't clear if this is caused by a bug or if it's an intended behavior), would love to get this out if ready.

norchow commented 6 years ago

I didn't have time to reproduce the issue again, and i suspect it was fixed by #164fbe3, so that line in question may no longer be needed.

If you think it's ready to merge, please go ahead 😄. We can take a deeper look into that line later.

gliechtenstein commented 6 years ago

Merged it in. Thanks for the amazing PR! https://twitter.com/_celljs/status/978746750327402496