search

intercellular / cell

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

Change some code to allow for a better api with virtual doms and such #112

Closed devsnek closed 7 years ago

devsnek commented 7 years ago

Changes how the initialization of cell is handled to primarily allow for server side rendering (see examples/server_side_render.js) and also for general nicer usage with cell + node

If anyone has a better idea for that Nucleus.tick getter please let me know

gliechtenstein commented 7 years ago

Wow this is great, I've started testing it out locally. First step to decoupling!

One question: could you discuss the Nucleus.tick part?

devsnek commented 7 years ago

Nucleus.tick was made a getter because the $root context might not be bound to the window at start time (when that self invoking function would run)

Making it a getter provides the same functionality (albeit slower the first call) and allows it to run correctly in contexts where the window isn't global at start time

gliechtenstein commented 7 years ago

Aha I see. Correct me if I'm wrong, but if this is because of the JSDOM call, couldn't we just move the +const cell = require('../cell'); line into beforeParse? https://github.com/intercellular/cell/pull/112/files#diff-6c15015da5ded0fecb2a92b3be15ac21R6

For example something like this (I haven't tried it yet so i may be wrong):

const { JSDOM } = require('jsdom');

const dom = new JSDOM('<html></html>', {
  beforeParse(window) {
    const cell = require('../cell');
    window.app = {
      $cell: true,
      $type: 'p',
      $text: 'hi!',
    };
    cell.create(dom.window);
  },
});

My concerns with using Object.defineProperty for Nucleus.tick are:

  1. I like the approach of passing $root as the argument of the self executing function, but I feel like assuming that $root may not always be bound yet beats the purpose of the self executing function and makes it non-deterministic.
  2. It would be best if we try to minimize the Object.defineProperty usage as much as possible, especially for Nucleus.tick, since this is the function that gets called the most throughout the entire library.

What do you think?

devsnek commented 7 years ago
  1. I was thinking about this a lot. The fact of the matter is A) in a browser we want it to run onload (if this.addEventListener) and B) in a not-browser we want it to wait until it is set by manually calling God.create. The current setup is what I came up with, but I am open to other methods of achieving this system.
  2. Moving the location of the cell require would not work (because the root scope would still not be window) and if it did I would consider it even more of a hack than the getter. the Object.defineProperty call only runs once, and the getter only runs once. I don't believe this will have an adverse effect on the performance of the lib
devsnek commented 7 years ago

@gliechtenstein i changed the code to remove the getter

gliechtenstein commented 7 years ago

@devsnek i've been actually thinking about this since last night after reading your comment, trying to see if there's a clean solution I can come up with. Does removing the getter work?

devsnek commented 7 years ago

@gliechtenstein yep it worked, passed all the tests. I forgot that if you just set it to the raw propery it defers until actually needed

gliechtenstein commented 7 years ago

@devsnek yesss awesome! OK I'm gonna play with this a bit more and then try to merge it tonight!

gliechtenstein commented 7 years ago

@devsnek OK Looks like it's good to go! Just one more thing before I merge:

should we also change https://github.com/devsnek/cell/blob/develop/cell.js#L254 and https://github.com/devsnek/cell/blob/develop/cell.js#L276 to use $root instead of window?

Was just going to merge and then do it myself with a separate commit but then thought I would ask in case you left them unchanged for a reason.

gliechtenstein commented 7 years ago

@devsnek Just wanted to get this taken care of today so went ahead and merged it, and made an additional commit to take care of the rest of the window instances https://github.com/intercellular/cell/commit/e42d30396f7209cbb937c169912271a9b1bb6cb7

Looks like it's working fine but let me know if you intentionally left them out, thanks!