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

WIP: separate create from mount #91

Merged
merged 1 commit into from Dec 4, 2016
Merged

WIP: separate create from mount #91

merged 1 commit into from Dec 4, 2016

Conversation

Swatinem
Copy link
Member

@Swatinem Swatinem commented Dec 1, 2016

This is a first step towards keyed iteration, see #81, #77, #33

The changes got a bit more invasive than I had hoped, but the tests do pass so thats a plus :-)
The EachBlock changes currently take the index as key, which is pretty useless for now, but pending some bikeshedding on actual keyed {{#each}} syntax, this should be good to go.

@codecov-io
Copy link

codecov-io commented Dec 2, 2016

Current coverage is 90.84% (diff: 100%)

Merging #91 into master will increase coverage by 0.08%

@@             master        #91   diff @@
==========================================
  Files            48         48          
  Lines          1310       1322    +12   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1189       1201    +12   
  Misses          121        121          
  Partials          0          0          

Powered by Codecov. Last update e09a39d...a80dcda

@Rich-Harris
Copy link
Member

Are we sure about doing keyed updates by default? It creates overhead (both in terms of code and computation) but in a lot of situations has no benefit. I think it needs to be opt-in – what does this look like with just the mount stuff, keeping the keyed updates for a separate tranche of work?

@Swatinem
Copy link
Member Author

Swatinem commented Dec 3, 2016

I rebased this with only the mount stuff, with a few other changes:

  • update does not require target, anchor anymore.
  • does initial mounting for things that are non-toplevel in if/each as well.

I am not so sure about the initial mounting for non-toplevel blocks however. It might interact strangely with components that have lifecycle callbacks. Those callbacks should only ever be called when the elements are actually mounted to the dom, so that getComputedStyle/offsetWidth and friends work properly. But that probably requires us to either fix #3 or make sure that component lifecycle events are only called on mount.
Let me see how far I can take that.

@Swatinem
Copy link
Member Author

Swatinem commented Dec 3, 2016

To elaborate further on the mindfuck I’m currently experiencing:

  • So we want to separate create from mount, to make moving fragments (keyed each) and dynamic yield possible.
  • We want to make sure that onrender hooks on components are called after the component is mounted, so that getComputedStyle/offsetWidth and friends work correctly. (not sure if this does work correctly right now at all)
  • We also want to mount non-toplevel children inside create, because we want to minimize the work the browser has to do on actual mount.
  • Child components use the same public component API, which means we can not separate between mounting into detached dom tree (because its a non-toplevel child), or mounting into the actual dom tree.

I think we somehow need to further split up the phases of mount to dom tree (might be detached) and fire onrender hook when the node is actually inside the real dom tree.
Ideas @Rich-Harris?

@Swatinem
Copy link
Member Author

Swatinem commented Dec 3, 2016

So there already was some onrender batching code present, although it was bugged :-)
Fixed that but found another bug that should be fixed by this PR \o/
Damn, this is all so heavily intertwined :-D

@Rich-Harris
Copy link
Member

Am working through the merge conflicts, will submit a PR to this branch shortly. Just curious about target and anchor here:

return {
  mount: function ( _target, _anchor ) {
    target = _target;
    anchor = _anchor;
    target.insertBefore( p, anchor );
   },

When would they be needed outside the mount method?

@Rich-Harris Rich-Harris mentioned this pull request Dec 4, 2016
@Rich-Harris Rich-Harris merged commit a80dcda into sveltejs:master Dec 4, 2016
Rich-Harris added a commit that referenced this pull request Dec 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants