-
-
Notifications
You must be signed in to change notification settings - Fork 404
renderComponent() #1099
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
renderComponent() #1099
Conversation
|
|
||
| When isInteractive is false, modifiers don't run, when true, modifiers do run. | ||
|
|
||
| #### `into` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could add after to allow app rendering after some node, without creating extra element wrapper
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've never heard of folks doing this -- is there a compelling reason / use case?
(maybe a bunch of render components in a list?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i kinda want to push back on this, as I don't know if the current render can do this.
It can always be added later if we need it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NullVoxPopuli, I agree.
Main use-case is render ember app in the middle of smf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
smf?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NullVoxPopuli in between nodes, for example, we have:
<html>
<body>
<header> ama header </header>
{ HERE_I_WANT_TO_RENDER_WHOLE_APP without creating extra node }
<footer> ama footer </footer>
</body>
</html>There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
I'm a big fan of this, as this is the baseline to write a storybook adapter for gts files. But I have some questions around this, as I think the narrative is not clear (yet):
|
aye! the
what do you mean no backing class? This is covered in RFC#931 import { template } from "@ember/template-compiler";
class Example extends Component {
static {
template(
"Hello {{message}}",
{
component: this,
scope: () => ({ message }),
},
);
}
}
I only included it because the sample implementation from @wycats included it -- I'm more than happy to remove it, as it does feel redundant, since you need a handle to that same element (it is the |
| /** | ||
| * Renders a component into an element, given a valid component definition. | ||
| */ | ||
| export function renderComponent( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wondering if it make sense to give function name just "render", because we can't render anything except component (or template-only component) in ember (app has it's own render logic)
text/1099-renderComponent.md
Outdated
|
|
||
| #### `into` | ||
|
|
||
| The element to render the compnoent into. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we have body as default target node?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we rename it to parentElement to make it match what the DOM has already defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we removed parentElement 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we have body as default target node?
idk, any downsides?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you removed the getter from the result. I see into is ambivalent. It can mean, that this is a parent or replaced with the contents you provide. parentElement is a concept, that is already known.
| const render = modifier((element) => { | ||
| let result = renderComponent(Demo, { | ||
| owner, | ||
| env: { document: document, isInteractive: true }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for non-interactive envs we may have different function like renderSSR, so, wondering if we really need isInteractive flag, any use-cases for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great question -- I'd need to dig in to glimmer-vm to see what all isInteractive is used for.
I don't know how much benefit there is to having renderComponent do multiple things via this flag vs your suggestion of renderSSR (which I like)
text/1099-renderComponent.md
Outdated
| * Destroys the render tree and removes all rendered content from the element rendered into. | ||
| */ | ||
| destroy(): void | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think render result should contain component instance itself, to be able to update it's properties if needed (and to be able to debug it)
|
A good question brought up by @void-mAlex at RFC review: what happens if you call |
update pushed! |
Co-authored-by: pomm0 <[email protected]>
|
Clarification has been added around the calling-multiple-times-on-the-same-element behavior, and during the RFC review meeting, since FCP is passed, we decided to merge -- I'll now head over to implementing -- yay! |
|
finished the implementation here: emberjs/ember.js#20962 |
Advance RFC #1099 `"renderComponent"` to Stage Released
| } | ||
| ``` | ||
|
|
||
| `RenderResult` is a subset of the currently a pre-existing `interface` from `@glimmer/interfaces`, but would be exposed as public API via the only the return type of `renderComponent`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| `RenderResult` is a subset of the currently a pre-existing `interface` from `@glimmer/interfaces`, but would be exposed as public API via the only the return type of `renderComponent`. | |
| `RenderResult` is a subset of the currently pre-existing `interface` from `@glimmer/interfaces`, but would be exposed as public API via the only return type of `renderComponent`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you pr this change? As this rfc is already merged (and implemented!) i can't apply this suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually didn't see the big bright Merged label at the top 🫠 ...
#1153
| /** | ||
| * When false, modifiers will not run. | ||
| */ | ||
| isInteractive?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will apply to other post-initial render things in the future
|
|
||
| `RenderResult` is a subset of the currently a pre-existing `interface` from `@glimmer/interfaces`, but would be exposed as public API via the only the return type of `renderComponent`. | ||
|
|
||
| It's shape is: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| It's shape is: | |
| Its shape is: |
|
|
||
| #### `env.isInteractive` (defaults to true) | ||
|
|
||
| When isInteractive is false, modifiers don't run, when true, modifiers do run. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"when this is true, modifiers will run. When this is false, modifiers will not run"
But why is it named isInteractive? Can't we name it runModifiers?
|
|
||
| ## How we teach this | ||
|
|
||
| `renderComponent` _is_ a low-level API, but it's use cases are powerful: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| `renderComponent` _is_ a low-level API, but it's use cases are powerful: | |
| `renderComponent` _is_ a low-level API, but its use cases are powerful: |
| ``` | ||
|
|
||
| > [!NOTE] | ||
| > Depending on your application, you may want to consider sanitizing user input, so users don't inject their own script / sytle tags into your app. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| > Depending on your application, you may want to consider sanitizing user input, so users don't inject their own script / sytle tags into your app. | |
| > Depending on your application, you may want to consider sanitizing user input, so users don't inject their own script / style tags into your app. |

Propose
renderComponent()Rendered
Summary
This pull request is proposing a new RFC.
To succeed, it will need to pass into the Exploring Stage, followed by the Accepted Stage.
A Proposed or Exploring RFC may also move to the Closed Stage if it is withdrawn by the author or if it is rejected by the Ember team. This requires an "FCP to Close" period.
An FCP is required before merging this PR to advance to Accepted.
Upon merging this PR, automation will open a draft PR for this RFC to move to the Ready for Released Stage.
Exploring Stage Description
This stage is entered when the Ember team believes the concept described in the RFC should be pursued, but the RFC may still need some more work, discussion, answers to open questions, and/or a champion before it can move to the next stage.
An RFC is moved into Exploring with consensus of the relevant teams. The relevant team expects to spend time helping to refine the proposal. The RFC remains a PR and will have an
Exploringlabel applied.An Exploring RFC that is successfully completed can move to Accepted with an FCP is required as in the existing process. It may also be moved to Closed with an FCP.
Accepted Stage Description
To move into the "accepted stage" the RFC must have complete prose and have successfully passed through an "FCP to Accept" period in which the community has weighed in and consensus has been achieved on the direction. The relevant teams believe that the proposal is well-specified and ready for implementation. The RFC has a champion within one of the relevant teams.
If there are unanswered questions, we have outlined them and expect that they will be answered before Ready for Release.
When the RFC is accepted, the PR will be merged, and automation will open a new PR to move the RFC to the Ready for Release stage. That PR should be used to track implementation progress and gain consensus to move to the next stage.
Checklist to move to Exploring
S-Proposedis removed from the PR and the labelS-Exploringis added.Checklist to move to Accepted
Final Comment Periodlabel has been added to start the FCP