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

Components created with a custom component manager cannot inject services #17236

Closed
jordpo opened this issue Nov 28, 2018 · 7 comments
Closed

Comments

@jordpo
Copy link

jordpo commented Nov 28, 2018

Because of the change made here, when using a custom component manager, the factory argument in the createComponent is not 'container aware'. So when you create the component instance and try to access a property or method on an injected service, you get an error => “Attempting to lookup an injected property on an object without a container, ensure that the object was instantiated via a container.”.

Here's a simple example of a component that uses a basic component manager. In this case, the error happens when you try to access the someString property, which depends on the intl service.

https://gist.github.com/patrickberkeley/6ebc14fab616b61ed4534d2ae588a5e9#file-component-js

Is this the intended behavior or should components created by a custom component manager have access to DI and injected services? If they should, is there an example of how to do that in this case?

@pixelhandler
Copy link
Contributor

pixelhandler commented Nov 30, 2018

@jordpo is setComponentManager public API?

Have you reached out in the community Discord channel or the forums? https://www.emberjs.com/community/

This issue seems like a question rather than a bug report, we typically use the issues here on github for addressing bugs. You may find more feedback from the community in the chat and/or forums on this topic :)

@patrickberkeley
Copy link

@pixelhandler yup as far as we know setComponentManager is public. Custom component managers were released in 3.4. The release notes and RFC both mention it (that's all the info we've found on component managers).

Yes, we have reached out on Discord in the #help room numerous times over the past few weeks regarding our questions on component managers. But have gotten little/no response. It seems like there are only a few (core?) people who have used or know about this functionality.

The reason we opened an issue here is that is what was requested by @rwjblue here: #16854 (comment). We are happy to use the appropriate forum whatever that may be. We are just hoping to get clarity on how to use component managers.

@jordpo
Copy link
Author

jordpo commented Nov 30, 2018

@pixelhandler Yeah I think this could be a documentation / clarity issue. It is public API but very cutting edge. I would imagine more people will get confused by this.

My original question / comment on the associated PR was why we are passing in the super class as an argument to createComponent instead of the component class / factory itself. By passing in the super class, you need to explicitly setOwner on the instance which is confusing as a required step.

@jordpo
Copy link
Author

jordpo commented Nov 30, 2018

Here's an example of this extra step that is required https://github.com/rwjblue/sparkles-component/blob/master/addon/component-managers/sparkles.ts#L29

@rwjblue
Copy link
Member

rwjblue commented Dec 7, 2018

In general, the changes referred to in the main descriptioin (
#16854) are correct and keep the implementation in line with the RFC itself.

I think the idea here would be that if the component itself needed/wanted access to the DI container, that the manager needs to pass it in to the constructor or explicitly state that the constructor cannot access these DI things (like the previous sparkles-component example you linked).

FWIW, I am working on a PR to sparkles-component to ensure that class fields and constructor in general do have access to DI things over in
rwjblue/sparkles-component#18 if you are interested in chiming in over there...

@jordpo
Copy link
Author

jordpo commented Dec 7, 2018

Thanks @rwjblue. I like the approach to explicitly pass in the owner and handle the setting in the constructor method on the component class. Not sure if we want to keep this issue open to track the documentation piece or if that will be covered in a separate issue. I'm fine closing it.

@rwjblue
Copy link
Member

rwjblue commented Dec 7, 2018

Sounds good, I'll go ahead and close...

@rwjblue rwjblue closed this as completed Dec 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants