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 access to service:-document in ember-engines #15129

Merged
merged 1 commit into from
Apr 27, 2017

Conversation

dnachev
Copy link

@dnachev dnachev commented Apr 11, 2017

Run into some problems trying to get some code to render server-side. I think the problem was related to ember-wormhole, although the addon currently doesn't have a good way to work in server-side at all, but it still required a reference to the document service to be able to initialize.

Although this can be fixed in ember-wormhole, I think it's good idea to allow components in engines to get access to the document if they need to and cover future problems.

Not sure whether enhancing the application instance test is enough, but not sure where to put a test, which says components in ember-engines must have access to the service:-document in applications, which uses ember-cli-fastboot.

@dnachev
Copy link
Author

dnachev commented Apr 11, 2017

Actually, on second thought, it may be worth to have a test in ember-engines to validate that a component has access to the document service in an engine instance. I don't think Fastboot plays a role in this fix. @trentmwillis what do you think?

@trentmwillis
Copy link
Member

@rwjblue does this seem like a reasonable fix? I'm not entirely sure what all the -document service is used for. Should Engines inherit it from the host or create their own?

@rwjblue rwjblue merged commit 837d17c into emberjs:master Apr 27, 2017
@rwjblue
Copy link
Member

rwjblue commented Apr 27, 2017

Yes, this is the correct fix. Sadly, I missed this PR when originally submitted 😢.

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.

3 participants