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

Skip portal rendering during SSR #331

Merged
merged 3 commits into from
Jan 12, 2018
Merged

Skip portal rendering during SSR #331

merged 3 commits into from
Jan 12, 2018

Conversation

robframpton
Copy link

@robframpton robframpton commented Jan 5, 2018

See #330 for more info.

See https://ui-metalportaldemo.wedeploy.io/ for a simple demo of the feature.

@robframpton
Copy link
Author

Documentation: bryceosterhaus/metaljs.com#91

@yuchi
Copy link
Contributor

yuchi commented Jan 9, 2018

Fantastic.

Copy link
Contributor

@jbalsas jbalsas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @Robert-Frampton, I'm still landing back, so it might take me a bit longer to wrap my head around this, but great job! Looks awesome!

I think I have just an initial question. What's the rationale behind the SSR logic for Portals? Can you explain the logic and expected behaviour?

@robframpton
Copy link
Author

Hey @jbalsas,

Right now it essentially skips the rendering of Portals during SSR, since Portals should be rendered to a specific element, and no such elements will exist at the time of running renderToString.

They could just be rendered inline like a regular component during SSR, but that would be counter productive since that markup will just be removed once the client side renders anyways.

@jbalsas
Copy link
Contributor

jbalsas commented Jan 11, 2018

Hey @Robert-Frampton, that's fine, I guess... I was reading how a kind of "universal portal" was implemented in https://michalzalecki.com/render-react-portals-on-the-server/

Other than that, I think the only part missing might be Event Bubbling. I don't think we've implemented it here... do we have plans for that? is it possible? Even if we don't implement it right away, it would be good to consider that case to make sure we can do that sometime soon. What do you think?

@robframpton
Copy link
Author

robframpton commented Jan 11, 2018

Hey @jbalsas,

Currently we don't have a mechanism for implementing that kind of even bubbling the way React does. The only thing we could do without creating a lot of new functionality is use our EventEmitterProxy to bubble the events to the parent Component instance, but inline event listeners (data-onclick="someHandler") won't pick up on that.

Do you think it's something worth pursuing?

Essentially, React is able to implement this behavior because they use a synthetic event system. https://reactjs.org/docs/events.html

@robframpton
Copy link
Author

It might be possible to implement something using our triggerEvent method. Normally we only use it for tests and not production code, but it's possible.

@robframpton
Copy link
Author

In order to do event bubbling the way React does it (without refactoring our event system), we’d essentially need to create an event listener for every event we want to bubble (creating a large whitelist of events), and then use dom.triggerEvent to dispatch those events to another element.

If we just wanted to bubble the events to the parent component, we could use DomEventEmitterProxy. The problem with that, is that it does not retroactively proxy event listeners that have already been added. So if a parent component adds a click listener during created, the child's DomEventEmitterProxy won't know to proxy it.

@jbalsas
Copy link
Contributor

jbalsas commented Jan 12, 2018

Looks like event bubbling might take us longer... I'm merging this since it already got several go aheads and we can keep thinking about this!

@jbalsas jbalsas merged commit 13530ca into metal:develop Jan 12, 2018
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.

3 participants