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

Feature Request: did-render modifier #568

Closed
boris-petrov opened this issue Jan 2, 2020 · 10 comments
Closed

Feature Request: did-render modifier #568

boris-petrov opened this issue Jan 2, 2020 · 10 comments

Comments

@boris-petrov
Copy link

This is somewhat related to #563. You can see I used to create classic components and then hook to didRender in order to get the HTML from the component (and do that over and over when the component rerenders due to something changing in it). Now I do something like:

{{#each dynamicComponents as |dynamicComponent|}}
  <span {{did-insert dynamicComponent.didInsert}}>
    {{~ component dynamicComponent.componentName model=dynamicComponent.model ~}}
  </span>
{{/each}}

Which works fine for the first render (because of did-insert) but doesn't when the component rerenders. What I did was to create my own modifier that uses native DOM APIs:

import { modifier } from 'ember-modifier';

export default modifier((element: HTMLElement, [handler]: [(element: HTMLElement) => void]) => {
  const observer = new MutationObserver(() => handler(element));

  observer.observe(element, { characterData: true, childList: true, subtree: true });

  return observer.disconnect.bind(observer);
});

However this is not a nice solution and perhaps the performance is much worse than a built-in did-render modifier that does what the didRender hook on classic components does.

There is a bit of discussion in Discord here.

@NullVoxPopuli
Copy link
Contributor

what's the use case for such dynamism?

@boris-petrov
Copy link
Author

It's funny how this is always the first question. 😆 There is more information in the linked RFC.

@pzuraq
Copy link
Contributor

pzuraq commented Feb 23, 2020

@boris-petrov I just published ember-render-detector. IMO this is the only way one should try to detect renders in the way you're describing. It's not something that should be common, since it effectively de-optimizes that entire render tree, but if it is necessary I think an escape hatch like this could probably exist for the forseeable future.

@boris-petrov
Copy link
Author

@pzuraq - thanks, I'll try this out! You're saying that this is going to be more performant than the modifier I wrote in the first post?

@pzuraq
Copy link
Contributor

pzuraq commented Feb 23, 2020

Modifiers are by design not capable of doing what you describe, and I think we want to keep it that way. They are only supposed to be concerned with a single element, they should not have any concerns about the sub-template.

@boris-petrov
Copy link
Author

Modifiers are by design not capable of doing what you describe

Well, the code I pasted does work - I've been using it for a while now. I agree that it's ugly though and that things like that are not semantically "right".

@pzuraq
Copy link
Contributor

pzuraq commented Feb 23, 2020

Ah, sorry, I thought you meant your proposed {{did-render}} modifier which would hook into the same systems that components used for the old didRender hook.

The modifier you wrote does not do that, and is a valid use of a modifier, but as you noted has performance issues since it uses a mutation observer which fires many times in a single render. <RenderDetector> exposes the didRender hook directly from a component, effectively, so it should be much faster. In the future, this could either continue to be a special capability that a single component such as <RenderDetector> opts into, or it could be a component added to the framework potentially.

@boris-petrov
Copy link
Author

Great, thanks a lot! I will try it out right away and will open issues in the project if I bump into any. Perhaps we can close this RFC then?

@pzuraq
Copy link
Contributor

pzuraq commented Feb 23, 2020

If you feel like this solves your needs, go for it 😄

@locks
Copy link
Contributor

locks commented Apr 18, 2020

Thanks for the discussion everyone! I'm closing the issue as it seems to have been resolved in a satisfactory way :)

@locks locks closed this as completed Apr 18, 2020
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

No branches or pull requests

4 participants