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

Fix wormhole insertion in FastBoot #96

Merged
merged 1 commit into from
Aug 1, 2017
Merged

Conversation

sandydoo
Copy link
Contributor

@sandydoo sandydoo commented Jul 5, 2017

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! 😄

@sandydoo sandydoo changed the title Fix wormholes in FastBoot Fix wormhole insertion in FastBoot Jul 5, 2017
@krisselden
Copy link
Contributor

krisselden commented Jul 27, 2017

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

@sandydoo
Copy link
Contributor Author

sandydoo commented Jul 30, 2017

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
Copy link
Contributor

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
Copy link
Contributor Author

sandydoo commented Aug 1, 2017

👍 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 simonihmig mentioned this pull request Aug 1, 2017
@simonihmig
Copy link
Contributor

Here are the FastBoot tests: #99! 🙂

@krisselden krisselden merged commit 1057950 into yapplabs:master Aug 1, 2017
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.

Renders in place in Fastboot environment
3 participants