Promote {{-in-element}} to public API #287

cibernox · 2017-12-22T12:42:58Z

Turbo87 · 2017-12-22T12:46:23Z

@cibernox are you aware of glimmerjs/glimmer-vm#619?

cibernox · 2017-12-22T12:49:10Z

@Turbo87 I knew it was public API in glimmer as it was mentioned in https://emberjs.com/blog/2017/10/10/glimmer-progress-report.html#toc_what-s-new-in-glimmer, but afaik it was not made public in Ember yet.

rwjblue

👌 I am in favor, thanks for putting this together!

lukemelia · 2017-12-22T14:57:57Z

I am in favor. It would be lovely to deprecate ember-wormhole, possibly preserving the name for a future generation of ember-wielding intergalactic travelers. 🚀

mehulkar · 2017-12-22T17:46:17Z

Could you point to any references to the existing private/intimate API for those of us who are not intimate with it yet so we can actually see what it is? :)

Serabe · 2017-12-22T19:13:42Z

@mehulkar https://emberjs.com/blog/2017/10/10/glimmer-progress-report.html#toc_portals

chancancode · 2017-12-22T19:20:04Z

@cibernox Thank you for putting this together! Can you expand the RFC a bit to not assume prior knowledge of the current private API?

Motivation: why would you need to use this, some example use cases, and why does this API solve those problems
Detailed design: what arguments does it take, what does it do, what happens when you pass fasley arguments, etc

The second point is especially important – enumerating the actual API surface helps avoid leaking "accidental" behavior into the public API, and also help reviewers identify potential problems.

Panman82 · 2017-12-22T20:29:55Z

What about having a helper that targets named outlets instead? Ex: {{#into-outlet name="foobar"}} Although I believe named outlets went the way of the Dodo, it seems more "ember-ish" than targeting a DOM element. This kinda feels like ember actions vs DOM events, (aside from the technical differences) one is the "ember way" and the other is the DOM way. Everyone knows what an outlet is and you wouldn't have to lookup the DOM element on a controller/component first. This would obviously require more thought (if it's even possible) but just an idea..

cibernox · 2017-12-22T20:37:22Z

@Panman8201 One scenario that your idea doesn't cover, is rendering content from inside an Ember app to anywhere else in the page. Even somewhere that is outside the Ember app (sometimes the app is not the entire page but only a section inside a static page)

cibernox · 2017-12-23T23:28:43Z

@chancancode I've added your suggestions.

chancancode · 2017-12-26T22:22:42Z

@cibernox looks great!

What happens when destinationElement is not falsy but not a DOM element? (what do we do today? it should probably assert in dev – at minimum we should explicitly state that it is not supported/undefined behavior)
What happens when destinationElement is a foreign element that belongs to a different document (e.g. in iframe?)? (I suspect this is probably an uncanny valley today, it probably "sort of works" but I bet it is not spec-compliant, or causes errors in some edge case – we probably want to disallow it)
What happens if you pass an element whose "context" is different than where you are now? For example:
```
<svg />
<body>
  <div>{{#in-element (jquery-select "svg")}}...{{/in-element}}</div>
</body>
```
Same thing when destinationElement is a <math />, <head />, <script />... etc. Does it work today? Do we expect this to cause problems? Should we disallow it for now? (I think one of the main use case is to render into title though.) /cc @wycats @krisselden @mmun @ef4
We should explicitly state that when you change destinationElement, the block is torn down and re-rendered (it calls the destroy hook on components, then create new ones at the new location, etc). This is perhaps not the ideal behavior, but it is how it works today. Maybe we should see if there is a way for us to carve out space in the proposal so that we can change this behavior in the future to moving the elements without tearing them down. (This comes with their own complexity, e.g. what if it changes from an HTML element to an SVG element or a foreign element?)
We should probably clarify that this appends to the end of the element (synchronously – is it problematic to guarantee that? cc @wycats). So {{#in-element e}}foo{{/in-element}}{{#in-element e}}bar{{/in-element}} should add foobar to e. There is a nextSibling argument that we support today to change this behavior, do we want to standardize that as well? If so, that comes with its own set of error cases – is falsy the same as null? what if nextSibling is not a direct children of destinationElement? what if it changes? etc.
I'm not sure how to say this – but "as usual" if you mutate the DOM yourself after rendering you might break subsequent re-render.

cibernox · 2017-12-27T09:43:24Z

@chancancode I updated the doc

Today a truthy value that does not "cuac" with ".appendBefore" throws an error. I believe that the behavior is correct, but the error message should me more helpful and only (HTML|SVG)Elements should be valid targets. Added to the rfc.
I didn't add anything on iframes, but it sounds like something we want to disallow with development assertions.
The rendered block should respect the context of where it is rendered. If a block is rendered inside an SVG element the content nodes should behave like children of SVG. Added to rfc.
Changing from HTML to SVG and vice versa should work. For what I've tested this already works because on each change of the destination element the entire block is torn down and rendered again as mentioned on the point above. Tearing everything down and rendering it again seems the safest behavior to me because of this. Added to RFC.
Right now it's how it works. Added to RFC unless someone sees a technical reason why this can't be guaranteed.
That is regular behavior in Ember and all frameworks I know. I did't see the point in mentioning as it's business as usual.

wycats · 2017-12-29T20:23:05Z

Like @chancancode, I am excited for this RFC.

For future readers, I think something like ember-elsewhere is a better fit for rendering components with data from one part of an Ember app into another part of an Ember app.

But there are plenty of cases (like the very common desire to render into <title>) that justify this API, and I was delighted to support it directly inside of Glimmer VM. Among other things, this means that the API is directly supported by the VM, and doesn't require custom DOM escape valves, which means it's yet another piece of common DOM work that can be modelled completely declaratively.

Let me try to answer @chancancode's questions, but these are just my personal answers (don't treat them as authoritative if you disagree 😉).

What happens when destinationElement is not falsy but not a DOM element? (what do we do today? it should probably assert in dev – at minimum we should explicitly state that it is not supported/undefined behavior)

I think this should assert in dev and fail silently in production. In general, the philosophy of Glimmer is to allow many dynamic questions to fail silently in production to keep apps running, while being very noisy in dev (and tests) to try to catch as many of these bugs as possible ahead of time.

I also think this should assert for any value other than null, or undefined, especially the falsy values "" and 0. I'm not sure how I feel about false, but it seems semantically strange and I don't intrinsically want to support it.

What happens when destinationElement is a foreign element that belongs to a different document (e.g. in iframe?)? (I suspect this is probably an uncanny valley today, it probably "sort of works" but I bet it is not spec-compliant, or causes errors in some edge case – we probably want to disallow it)

I think we should disallow this until we have a separate RFC that covers how we should deal with foreign elements in general. Should the foreign context adopt the element? Would that cause other issues? How would either solution play with SVG?

What happens if you pass an element whose "context" is different than where you are now? For example:

There are two categories of "contexts" in HTML that are relevant to Glimmer:

Namespaces, which affect the rendering of content even when the element is created using createElement. This means that if you put a document.createElement('g') into an <svg> element, it will not render as expected because it has the wrong namespace.
HTML parser contexts ("in body", etc), which primarily cause elements to be ignored or moved during parse-time.

Today, we are very careful about namespaces, so if you put a block with <a /> inside of an <svg> element, we will correctly create the <a> tag with the correct (SVG) namespace. For namespace purposes, Glimmer treats template stitching as an extension of the HTML parser, and carefully follows the parser's rules as if the whole content was parsed as a single piece of HTML (this includes things like correct handling of <foreignElement> and casing rules).

On the other hand, we do not (currently) reject stitching together content because the child content is not allowed by the HTML parser. In practice, this typically doesn't cause problems, with one major exception. In SSR mode, Glimmer serializes its DOM into HTML, which is then parsed on the client. If the parsing fails (for example, if a piece of content is ignored or moved), the behavior will differ between client-side rendering and SSR + rehydration.

This is a real issue and one that we should address systemically, but I don't think we need to address it in this RFC to make progress. When we do address it, we will need to handle moving a <p> into a <tr> via {{in-element}} the same way that we will need to handle a {{yield}} producing the same result. If past is prelude, it will probably be deprecated slowly and carefully, with a noisier deprecation in SSR mode.

We should explicitly state that when you change destinationElement, the block is torn down and re-rendered (it calls the destroy hook on components, then create new ones at the new location, etc). This is perhaps not the ideal behavior, but it is how it works today. Maybe we should see if there is a way for us to carve out space in the proposal so that we can change this behavior in the future to moving the elements without tearing them down. (This comes with their own complexity, e.g. what if it changes from an HTML element to an SVG element or a foreign element?)

Teardown + rerender are today's semantics, and those semantics avoid thorny (design and implementation) issues that imo we shouldn't try to resolve today.

I think the RFC should be explicit about today's semantics, including the execution of destroy hooks, and that if we want to enable "move" semantics, we should do so in a more holistic way that would also work for moving components between lists and other desirable "move" scenarios.

We should probably clarify that this appends to the end of the element (synchronously – is it problematic to guarantee that? cc @wycats). So {{#in-element e}}foo{{/in-element}}{{#in-element e}}bar{{/in-element}} should add foobar to e. There is a nextSibling argument that we support today to change this behavior, do we want to standardize that as well? If so, that comes with its own set of error cases – is falsy the same as null? what if nextSibling is not a direct children of destinationElement? what if it changes? etc.

I think we can get away with making it append-only for now (to avoid dealing with the cases you're describing). The only question is whether we want append vs. replace. For example, in the <title> scenario, you can imagine someone putting a placeholder value in the DOM so that something shows up regardless of whether the {{in-element}} is hit during initial render (and also while model data is loading).

I think "replacing the entire content" is probably the majority use-case, so maybe what we want to do is make replace the default but also allow an position="append" option, which would could later expand to support nextSibling if needed.

(in most cases, people could work around the nextSibling limitation by creating a wrapper element -- it's not usually good enough, but I suspect that for the use-cases targeted by this RFC, it would be managable for the time being).

I'm not sure how to say this – but "as usual" if you mutate the DOM yourself after rendering you might break subsequent re-render.

We probably should have a formal description of this usual caveat. It amounts to a somewhat clear technical description around the details of bounds markers (and similar attribute concepts) in the Glimmer implementation, but we would want to turn those limitations into something understandable for humans.

For this RFC, it's probably acceptable to gesture towards "similar limitations as manually mutating a component's DOM".

simonihmig · 2018-01-02T09:44:04Z

The following is related, but probably should not prevent progress of this RFC. I have been playing with this a bit recently (see https://github.com/kaliber5/ember-in-element-polyfill), and what I find in particular difficult is to get this to work properly with FastBoot. The problem is that destinationElement expects a DOM node, which is not so easy to get a reference for when running in FastBoot. ember-wormhole has the same problem, and solves it by

getting access to document (in case of FastBoot the simple-dom instance) through private APIs, see https://github.com/yapplabs/ember-wormhole/blob/f6bdea79ee1d9b3ef6b7f19e378e3bfb103b2e5e/addon/utils/dom.js#L47-L63
polyfilling document.getElementById (which is not available in simple-dom), see https://github.com/yapplabs/ember-wormhole/blob/f6bdea79ee1d9b3ef6b7f19e378e3bfb103b2e5e/addon/utils/dom.js#L26-L43

So for this to become easily usable in FastBoot I think it would be useful to either

have a public way of getting the document reference (simple-dom) and some sort of DOM traversal support in simple-dom, at least getElementById, or even querySelector (see Implement querySelector in Document ember-fastboot/simple-dom#53)
or extend the in-element API to also accept an ID or even a full CSS selector as destinationElement, with a FastBoot-compatible implementation that hides the above mentioned nasty implementation details from the user.

Gaurav0 · 2018-01-02T16:02:46Z

Another issue is that ember wormhole currently requires you to use renderInPlace in acceptance tests. Is it possible to come up with a way to fix this?

cibernox · 2018-01-02T16:05:20Z

@Gaurav0 ember-wormhole does not require to use renderInPlace in tests, ember-power-select used it (until 2 weeks ago) and it works well, as long as the destination is on the page.

ef4 · 2018-01-02T16:47:29Z

@simonihmig you're quite right that needing access to an actual DOM node makes this API hard to use. In addition to your fastboot example, it's just fundamentally timing-dependent in a tricky way.

But I also agree that's not a problem this rfc should solve. I don't think it's even a problem glimmer should solve, because it would introduce a lot of complexity into a layer that doesn't need to have it. in-element is the smallest low-level primitive that lets us build solutions on top of glimmer without delving into internals.

I would like to continue making ember-elsewhere be a pleasant high-level API. It should adopt in-element as implementation, but also hide the timing dependency for you (so that you can render source and destination in either order, or even put the destination inside a conditional and turn it on and off). All that is easier done in normal "userspace" ember using a service to coordinate.

wycats · 2018-01-02T21:05:03Z

@cibernox Can you update the RFC with some of the information in my comment (if you agree with it) or raise questions (if you don't).

I think it would be great to FCP this sucker, but @chancancode is right that we need to get the answers to his questions into the text of the RFC.

cibernox · 2018-01-02T23:14:42Z

I updated the RFC. The only bit I'm not convinced about (or I misunderstood) is the append vs replace.

I believe that is not that uncommon to want to wormhole two thing in the same destination. Example: Two dropdown opened simultaneously (not all dropdowns are mutually exclusive).

Right now this seems to work with {{-in-element}}, so I'd like to keep it that way at least by default.

wycats · 2018-01-03T00:34:25Z

@cibernox the concern that @chancancode had, which I want to reiterate, is that supporting nextSibling requires us to specify the behavior for various values of nextSibling, like null, undefined, strings, etc. It requires us to deal with nextSibling that isn't a child of the destination element as well as deal with what happens if it changes.

I was thinking in the near term it would be fine to just avoid those issues by going with replace semantics for now. Alternatively, if you think the semantics are important, you could figure out how it works now (in terms of all of those questions) and document it in the RFC 😄

simonihmig · 2018-01-03T01:03:04Z

I believe that is not that uncommon to want to wormhole two thing in the same destination.

I would want to second that! So this would require append-semantics, rather than replace.

FWIW, ember-cli-document-title does not use -in-element, but a few addons use it already today for dropdowns, tooltips or modals, to move the overlay to the top of the DOM tree, e.g. to prevent potential clipping issues with overflow: hidden by ancestor DOM elements. Examples: ember-basic-dropdown, ember-bootstrap, ember-popper. For these use cases append-semantics are required (those UI elements are not mutually exclusive), which should be the default semantics IMHO. (and as far as I'm concerned the only one, at least for now)

Regarding the nextSibling thing: I don't feel a strong need to have this now, as part of this RFC...

rwjblue · 2018-06-21T16:38:25Z

Nothing was really preventing addons from experimenting with the private api

The blockers are fairly clear actually:

lack of documentation
risk of using private APIs that have no SemVer guarantee

Clearly from @cibernox's example (and others) some addon authors have discovered the "magical incantation" and are willing to accept the risk, but that is far from "nothing". 😸

I believe a bit of sugary layer would make a huge difference, especially for new users, otherwise they would have to learn how to create their own helper or adhoc solution to use this feature.

Yep, as mentioned before, I totally agree and is a nice incremental (concept wise) step forward. There is no reason to avoid adding the lower level API now, and adding a better high level API later.

chancancode · 2018-06-22T18:41:29Z

To be very clear – we don't think anyone should be replacing the addons they are using today with this new API (perhaps the "Motivation" part of the RFC should be more clear about this). The only goal is that there are addons that uses this today and we would like to offer them more stability guarantees (and transitively, to the users of those addons) than today.

An API that takes DOM elements are more "primitive" than selectors in several sense:

This is a Glimmer VM feature which has to work in Fastboot environments that does not have querySelector etc
As other pointed out selectors only work for things that are addressable (with ID, unique classes, etc) and doesn't work for, e.g. elements that are detached from DOM (maybe you want to render into an element an element outside of the DOM tree to collect the innerHTML, etc)
Edge cases like: what if the selector refers to more than one element, or that the element pointed to by the selector changes

100% agree that this API is not convenient enough for end-users to adopt, but that's why this is a primitive API for other addons to build on top.

rwjblue · 2018-06-22T18:54:05Z

We discussed this at todays core team meeting, and are still in favor of moving forward. We are also very much interested in further RFCs in this space (e.g. possibly a more user-facing proposal like {{render-in 'some-selector'}}).

Thanks to everyone who participated in this RFC!

0xadada · 2018-06-25T13:50:41Z

I just used {{-in-element}} to implement a modal window component, and this would give me that warm fuzzy feeling about putting it into multiple production projects.

Promote {{-in-element}} to public API

5667872

Improve comment about deprecation

1208d56

rwjblue reviewed Dec 22, 2017

View reviewed changes

Turbo87 approved these changes Dec 22, 2017

View reviewed changes

chancancode self-assigned this Dec 22, 2017

cibernox added 2 commits December 23, 2017 23:30

Apply suggestions

8b8e9a7

Update notes about behavior

64946b7

cibernox force-pushed the make-in-element-public branch from 7663a1c to 64946b7 Compare December 23, 2017 23:27

Save clarifications

e39be19

Update RFC

25a013d

rwjblue merged commit 219860c into emberjs:master Jun 22, 2018

cibernox deleted the make-in-element-public branch June 26, 2018 19:37

lougreenwood mentioned this pull request Jul 15, 2018

Adding support for use with ember-wormhole poetic/ember-cli-photoswipe#14

Open

btecu mentioned this pull request Aug 15, 2018

in-element helper not working emberjs/ember.js#16886

Closed

knownasilya mentioned this pull request Jan 6, 2019

Deprecate Route Render APIs #418

Merged

chadhietala mentioned this pull request Feb 11, 2019

[FEATURE | BREAKING] Change semantics of in-element to match emberjs/rfcs#287 glimmerjs/glimmer-vm#918

Merged

kategengler mentioned this pull request Feb 12, 2019

RFC #0287 - Tracking for Promote {{-in-element}} to public API emberjs/rfc-tracking#25

Closed

7 tasks

josemarluedke mentioned this pull request Aug 12, 2020

Add in-element to built-in helpers embroider-build/embroider#506

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Promote {{-in-element}} to public API #287

Promote {{-in-element}} to public API #287

cibernox commented Dec 22, 2017 •

edited

Loading

Turbo87 commented Dec 22, 2017

cibernox commented Dec 22, 2017

rwjblue left a comment

lukemelia commented Dec 22, 2017

mehulkar commented Dec 22, 2017

Serabe commented Dec 22, 2017

chancancode commented Dec 22, 2017

Panman82 commented Dec 22, 2017

cibernox commented Dec 22, 2017

cibernox commented Dec 23, 2017

chancancode commented Dec 26, 2017

cibernox commented Dec 27, 2017

wycats commented Dec 29, 2017

simonihmig commented Jan 2, 2018

Gaurav0 commented Jan 2, 2018

cibernox commented Jan 2, 2018

ef4 commented Jan 2, 2018

wycats commented Jan 2, 2018

cibernox commented Jan 2, 2018

wycats commented Jan 3, 2018

simonihmig commented Jan 3, 2018

rwjblue commented Jun 21, 2018

chancancode commented Jun 22, 2018

rwjblue commented Jun 22, 2018

0xadada commented Jun 25, 2018

Promote {{-in-element}} to public API #287

Promote {{-in-element}} to public API #287

Conversation

cibernox commented Dec 22, 2017 • edited Loading

Turbo87 commented Dec 22, 2017

cibernox commented Dec 22, 2017

rwjblue left a comment

Choose a reason for hiding this comment

lukemelia commented Dec 22, 2017

mehulkar commented Dec 22, 2017

Serabe commented Dec 22, 2017

chancancode commented Dec 22, 2017

Panman82 commented Dec 22, 2017

cibernox commented Dec 22, 2017

cibernox commented Dec 23, 2017

chancancode commented Dec 26, 2017

cibernox commented Dec 27, 2017

wycats commented Dec 29, 2017

simonihmig commented Jan 2, 2018

Gaurav0 commented Jan 2, 2018

cibernox commented Jan 2, 2018

ef4 commented Jan 2, 2018

wycats commented Jan 2, 2018

cibernox commented Jan 2, 2018

wycats commented Jan 3, 2018

simonihmig commented Jan 3, 2018

rwjblue commented Jun 21, 2018

chancancode commented Jun 22, 2018

rwjblue commented Jun 22, 2018

0xadada commented Jun 25, 2018

cibernox commented Dec 22, 2017 •

edited

Loading