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

Removed the public version of the -portal service #9

Merged
merged 1 commit into from
Aug 3, 2020

Conversation

cah-brian-gantzler
Copy link
Contributor

@cah-brian-gantzler cah-brian-gantzler commented Jul 1, 2020

Not sure you were aware you can do this, but it removes the public export of the service. You also could remove the - in the service name, since it is no truly private.

Note you could also remove the component exports as will and change the documentation to

<EmberStargate@Portal>

Also avoiding name conflicts.

I have been moving towards this as it clearly indicates the addon the component comes from and is a good start towards template imports.

Can do another PR if you would like to see the components working in your tests as well

@simonihmig
Copy link
Owner

Not sure you were aware you can do this, but it removes the public export of the service.

That's great. Tbh never have seen this before. Do you happen to have some link at hand where this is documented? Is this public API and guaranteed to stay?

@cah-brian-gantzler
Copy link
Contributor Author

I can not find documentation for this. I had a discord discussion with Robert (rjwblue) about it and he confirmed this was public and was going to continue to be supported. I believe this may be part of they underlying that will make template imports work.
If you look at https://github.com/miguelcobain/ember-yeti-table we converted all the internal private components to use the template namespace

@simonihmig
Copy link
Owner

If you look at https://github.com/miguelcobain/ember-yeti-table we converted all the internal private components to use the template namespace

That's an excellent use case, and I would really like to use this pattern (e.g. ember-bootstrap has a lot of private (yielded) components). But tbh I would have expected this to be introduced in an RFC if it were really public, not? Also would like to know since which Ember version this is supposed to work (obviously it works in all those that this addon supports at least)...

@rwjblue mind chiming in and bring some light into this? 🙏 😀

@cah-brian-gantzler
Copy link
Contributor Author

I dont remember the version, but it worked back way farther than I thought, was surprised. Yes I hope Robert sheds some light on when they may be documented since it has been working for a long time

@rwjblue
Copy link

rwjblue commented Jul 22, 2020

Ya, it is a public API of ember-resolver (not Ember itself). When attempting to resolve something it parses the @ as meaning "this namespace" (remember back in the day when we had window.Foo style namespaces and you could refer to them, it is basically the same thing).

@rwjblue
Copy link

rwjblue commented Jul 22, 2020

Its generally worked since at least 2016

@rwjblue
Copy link

rwjblue commented Jul 22, 2020

@rwjblue
Copy link

rwjblue commented Jul 22, 2020

FWIW, the feature was added in ember-cli/ember-resolver#70 (2014).

@cah-brian-gantzler
Copy link
Contributor Author

cah-brian-gantzler commented Jul 22, 2020

@rwjblue Thanks for all the clarification.

The other question was, is it documented? (Since its a little known feature) and should we be teaching this?

@rwjblue
Copy link

rwjblue commented Jul 22, 2020

I dunno if it is documented, I'd be happy to land a PR adding docs to ember-resolver. To document more broadly we'd have to discuss a bit more (for example, over in the cli-guides repo).

IMHO, the main thing here is to ensure that service names are namespaced to avoid collisions, you can use this @ thing for that or you could make your services -ember-stargate-portal both (AFAICT) solve the problem.

Copy link
Owner

@simonihmig simonihmig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good to merge then!

Thanks @cah-briangantzler for the work, and @rwjblue for the insights! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants