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

move render logic out of node objects #1746

Merged
merged 38 commits into from
Sep 24, 2018
Merged

move render logic out of node objects #1746

merged 38 commits into from
Sep 24, 2018

Conversation

Rich-Harris
Copy link
Member

This is a step in the direction of #1678. Remains to be seen if making nodes no longer be 'self-rendering' will end up introducing too much complexity. So far I think I prefer it, but I've only moved SSR logic so far.

@Rich-Harris
Copy link
Member Author

Wrestling with this a bit. The SSR stuff is fairly easy, but the DOM renderer is trickier to break out, because it requires two passes. The first pass does things like figure out which subtrees can be represented with static HTML (if you try to do that at the same time as you're generating the code, you can end up doing a lot more graph traversal) and assigning variable names to nodes (though that could be done inside the Block class). The second does the actual rendering.

The result of that initial pass is stored in the node objects themselves, which is very convenient. But it doesn't really make sense if the nodes are supposed to become renderer-agnostic abstract objects.

So there are a few options:

  • Abandon this PR. I don't like that idea, since keeping render logic inside nodes inhibits the kind of experimentation that was the original motivator
  • Keep DOM render logic inside nodes, keep SSR render logic separate. Would be a bit weird
  • Push forward with what will probably turn out to be a fairly chunky refactor

I'm still leaning towards 3... though it might get messy.

@Rich-Harris
Copy link
Member Author

Hell of a lot of work to do here still but I figured I'd check in what I've got so far. Basically, am creating 'wrappers' around the various markup node objects, with the render logic that is currently in those objects. Things are messy right now, but I think this will make more sense once it's finished — it will be much clearer which bits of code relate to which phase of the compilation.

It does mean creating more objects, which in theory is going to make compilation slightly slower. I'm not anticipating that it'll be a meaningful amount though — we'll see.

@Rich-Harris
Copy link
Member Author

This is a real slog but I'm almost there. Definitely satisfied that it was the right move — it's allowed me to change some of the bits of the codebase I disliked the most, e.g. all the finicky DOM node binding logic.

@Rich-Harris Rich-Harris changed the title [WIP] move render logic out of node objects move render logic out of node objects Sep 23, 2018
@Rich-Harris
Copy link
Member Author

There's definitely some more work that could be done here to tidy things up, but I think it's probably at a point where it's better to merge it and do any additional work in follow-up PRs, since the high-level structural stuff is basically done

@Rich-Harris Rich-Harris merged commit 1564553 into master Sep 24, 2018
@Rich-Harris Rich-Harris deleted the gh-1678 branch September 24, 2018 02:47
@Rich-Harris
Copy link
Member Author

ah dammit i meant to squash this branch

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.

1 participant