search

yapplabs / ember-wormhole

Render a child view somewhere else in the DOM.
MIT License
284 stars 68 forks source link

Fix wormhole insertion in FastBoot #96

Closed sandydoo closed 7 years ago

sandydoo commented 7 years ago

Fixes #86 – wormholes are rendered in-place in FastBoot.

Since willRender is not available in FastBoot, use didReceiveAttrs instead to schedule _appendToDestination.

Is there some technical/performance reason why this shouldn't be done? I've tested this fix in our production app which uses wormholes extensively. Would love to hear some thoughts on this! 😄

krisselden commented 7 years ago

Is there a way to test this? This could just be scheduled in init.

sandydoo commented 7 years ago

Yes, that's even better and seems to work (updated to use init now). The question I have is whether the previous approach (using willRender with a flag to run it once) was introduced on purpose. The comment left by @bantic:

didInsertElement does not fire in Fastboot. Here we use willRender and
a _didInsert property to approximate the timing. Importantly we want
to run appendToDestination after the child nodes have rendered.

As for testing in fastboot, since there isn't any fastboot testing implemented here, I wouldn't be comfortable pursuing it without a maintainer approving the approach. I'm even not sure what the accepted approach is these days. ember-fastboot-addon-tests or something custom?

simonihmig commented 7 years ago

Thanks @sandydoo for working on this. Would love to have this finally fixed!

Regarding tests, I think we can/should add those anyway. The tests will just assert if the outcome is right, regardless of the actual implementation.

Using ember-fastboot-addon-tests they should be pretty straightforward (hope so!). I could even contribute those tests if wanted, probably even as a separate PR!?

sandydoo commented 7 years ago

👍 Yes, fastboot tests are a must to avoid this kind of regression in the future. If you've got the time to add the tests in a separate PR, that would be great! In theory, the most basic test that it renders in a remote div should fail until this PR is also merged.

simonihmig commented 7 years ago

Here are the FastBoot tests: #99! 🙂