-
-
Notifications
You must be signed in to change notification settings - Fork 408
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
Nested Invocations in Angle Bracket Syntax #457
Conversation
Thanks for working on this @wycats! The inability to invoke components in subdirectories has been a significant blocker to adoption of angle bracket invocation for many people. Its great to finally see work to make a migration possible that doesn't require fundamentally restructuring your app. While I can absolutely see how folks coming from languages that use In addition, the ember-holy-futuristic-template-namespacing-batman addon (which allows using tldr; I disagree with your assesment of |
I'll second that we also see lots of folks not adopting angle brackets due to lack of subdirectory support, so I'm glad this is being prioritized! IMO the timeline for template imports has a big impact on the need for an intermediate solution like this. Adding another "Emberism" like ES6 imports have been used for quite some time both inside and outside the Ember community, and I have yet to hear a JavaScript developer complain about a need to abstract filesystem paths. The syntax used for absolute and relative paths is a familiar, battle-tested way to reference and look up modules on disk. The problem space for resolving component invocations in templates seems to sufficiently overlap with other environments that the benefits for a new syntax would need to be high to justify their cost. Template imports seem to solve the general problem, so unless there's been an assessment of high risk in shipping those, I think I'd rather community effort allocated towards that RFC. |
@samselikoff - Yes, you are absolutely correct that both this RFC and the general "template imports" problem are related. I think we are generally all aligned on that future, but given that we are only just now talking about exposing primitives that can be used outside of core to build these template imports (checkout #454 if you haven't already) it seems very unlikely that a concrete "template imports" implementation will be ready in the time frame of the octane edition. This RFC (once corrected to use |
@rwjblue Ah - I see. Thanks for clarifying! 😀 In this case, I guess another direction we could consider would be to take the "do nothing" alternative that was mentioned in the RFC a bit further, and simply punt on promoting Angle Bracket invocation altogether until it has support for nested components via template imports. After all, if template imports is the goal, why introduce a new short-lived syntax? Why not just stick with curlies for now, if we know what we really want is being worked on & near the top of the roadmap? The introduction of The reality is that nested-lookup-angle-brackets and template-import-angle-brackets are two different things for folks to learn, so I'm not sure this actually makes the teaching story easier. |
To be fair, nested lookup is something that is already used with curly bracket components, so it's well understood by the community. That should reduce the teaching cost a fair amount. I think that this would allow us to bundle a lot of the "frontend" concepts together all at once, teaching the new Glimmer component API, tracked properties, angle bracket syntax, named args, etc. These are all highly interrelated, and so they really rely on each other for coherence in the learning story. This way we can focus on teaching these while we work on template imports, and get things started for them instead of continuing to wait for all the things to be complete |
I do think the concept of nested lookup is already used and understood, however the syntax for the three forms is quite different and will all need to be explained. Form 1: {{#budget/category/table-row}}
...
{{/budget/category/table-row}} Form 2: <Budget::Category::TableRow>
...
</Budget::Category::TableRow>
<!-- Or this: -->
<Budget/Category/TableRow>
...
</Budget/Category/TableRow> Form 3: import CategoryTableRow from './table-row';
// Or this:
// import CategoryTableRow from 'money-manager/ui/budget/category/table-row';
---
<CategoryTableRow>
...
</CategoryTableRow> Forms 2 and 3 both need to be taught. I agree they can all be learned by Ember developers; my concern is just that when the day comes when we all move from Form 2 to Form 3, there'll be some mental churn as well as code changes that will need to happen to accommodate. There will be a period of time where there are many ways to invoke components, which feels a little unnecessary and confusing to me. Related question: Do y'all have any insight on what lead to #454 being about primitives instead of the new final format? Was there just a lack of consensus during the exploratory phase? IOW, can the core team just grab some 🍻 and make a call on the final template import syntax? 😜 I feel like if there was less uncertainty about what that final syntax would be, we probably wouldn't even be discussing a temporary interim syntax. |
So for us, a lack of a way to easily use nested component paths is the main reason we cannot really move to Angle Bracket Invocation. This kind of leaves us in a weird space, where we have to kind of use both syntaxes to achieve certain things. Honestly, I'd definitely prefer it if we could (internally) just settle on "use angle bracket invocation for all new code", but that's just not possible (since we have a lot of existing components in nested folders, which we quite like), so we end up in a weird place where you always need to think about which variant to use. For me, the |
@samselikoff I believe the motivation for RFC'ing primitives instead of a final import syntax was the idea that the former would allow for exploration with various syntaxes in addon space and presumably a more complete feedback cycle for the core team prior to making a final decision about new semantics and syntaxes. I don't think that's a bad idea but of course it's subjective. Octane is already available for (experimental) preview using the blueprint and I'm guessing a more formally published preview is on track for Ember Conf. I personally think it would be unwise to rush a new core feature with both syntactical and semantic changes in those kinds of timelines. Also very subjective, but when I first heard about angle bracket invocation about a year ago I just assumed that nested lookup would have been supported with the syntax being advocated for by @rwjblue and others (slashes), i.e. I believe it would be a very intuitive and therefore low cost adoption for all Ember developers because there's no change to the existing mental model. |
@sohara I agree rushing such a core feature by EmberConf would be a mistake. I was under the impression Octane is not going to be finalized as of the EmberConf deadline, but rather when there is cohesion around all the new moving parts. If a core piece of one of those new moving parts isn't fleshed out, I don't see the need to rush the others. (Of course, deadlines can and should be used to force scoping down, without a loss of cohesion.) I also am in agreement that angle-brackets-with-slash-syntax is intuitive and relatively low-cost on its own. The problem I'm having is introducing it as a temporary solution, since nothing about it suggests as much. |
Another possible alternative design to the one proposed here (either Concretely, I'd propose adding a {{resolve-component 'app-icons/warning' as=AppIconWarning}}
<AppIconWarning @color="yellow" /> This has a few benefits:
h/t @mixonic for originally mentioning this idea in the Octane Meeting yesterday... |
I really, really do not want to turn the comments here off the rails entirely. Please, if you want to experiment with syntax specifics join us on Discord and do it there ❤️ #dev-ember-js or #st-octane are appropriate. I would however like to clarify that I don't prefer I would suggest: {{lookup Warning in 'app-icons/warning'}}
<Warning @color="yellow" />
Again please help us keep the conversation focused for this design and discuss tweaks and exploratory ideas on Discord ❤️ Thank you. |
Do {{#let (component "app-icons/warning")
as |Warning|
}}
<Warning />
{{/let}} |
@ro0gr - no, the idea is that they would transform into that syntax |
Some initial impressions I have:
Reactions to comments so far, I think if we are going to do something to allow better semantics than having to write lots of |
As someone with an app full of nested components I'd rank my preferences as:
I don't find |
My team recently converted a couple apps with plenty of nested and dynamic components to use Angle Bracket syntax. We opted to use I think what most people need right now isn't a learning story or migration path to future features. They need a way to provide consistency to their code. They need an answer to Angle Bracket invocation for nested components that doesn't require a blog post. If template imports aren't actually coming in Octane, then the Personally, I would gladly take I'd rank my personal preferences as
If I attempt to empathize with devs not reading RFCs, but just googling and reading docs when running into issues, I'd rank my theoretical preferences as
|
I want to try to clarify something. The We could have designed a syntax for nested lookups in that RFC, but intentionally chose not to because we felt, at the time, that Module Unification would replace the need for that syntax:
In the interim, we have come to believe that template imports, rather than local lookup in Module Unification, is the correct solution for bringing nested components into the scope of a component. However, the template imports feature is not yet RFCed, while angle bracket components have already shipped. In light of that, this RFC suggests revisiting the decision made in RFC #311 to punt on designing a syntax for nested lookups. It is narrowly scoped to introducing a syntax for that purpose, in order to allow users attempting to use already landed angle bracket syntax today to migrate to them uniformly from the curly syntax. We intend to continue working on template imports, but I personally believe that we need a fully general way to use angle bracket syntax while we design, implement and ship template imports. |
Another thing I want to note: the In particular, it would require us to introduce the concept of bringing external components into scope, but it would only be necessary in nested scenarios. This introduces some questions: should people use the same syntax for bringing in non-nested components? Should we recommend that? Will community members want it to be recommended? While folks this thread might feel pretty good about introducing an import-like API to Glimmer templates, it really does introduce a big new thing to teach: the concept of bringing external templates into scope. I don't think it will make sense to teach it as a desugaring to In contrast, this RFC maintains the current mental model around using name resolution to get components out of the file system, but adds a way to refer to nested directories. I just think that's a smaller scoped thing to add and teach. |
For me, the argument that a) Refactor your whole codebase to not use nested components For me, all of these options are definitely worse then Also, it has been said before, but I really think that it is not unreasonable for a regular developer to just expect that, if Basically, I'd say that it is harder to teach that (I'd prefer |
For me, Octane is great because it will bring clarity, readability and coherence. This is why I really like angle bracket invocations. So, I don’t like a path where we go back to curly invocation. And I don’t like a mixed solution: angle bracket should be used everywhere for direct component invocation (do we have a plan to deprecate curly yet?). We stared to migrate to angle bracket invocation and we love it! But we need a solution for our numerous nested component. The Meanwhile, I don’t see the Additionally, the Finally, I foresee a lot of problems with the I’m in favour of the new funky |
Me too and I guess most of the folks prefer
The syntax highlighter and autocompletion look to be the blocker for adopting Anyway, I think if syntax highlighters and IDE autocompletion start widely supporting I am not sure about the Have <AppIcons:Warning></AppIcons:Warning> Probably, the Anyway, I am excited to see progress on both initiatives! |
Since module unification still is not completely finished we decided to use nested components to structure our code better. So the described problem also hits us. Another problem with the If we need to change the But I'm not sure what's the migration idea when module unification and/or template imports are released. When we need to adjust the syntax of component invocation by hand the Also, I agree with the syntax highlighting problem described in the RFC for But for me, it's most important to have a solution which can be easily changed to the future syntax. |
My perspective on First, it would require us to update Second, it would require updates to all of the syntax highlighters in common use in Ember. This is harder than it looks, because Ember highlighters typically use Handlebars syntax, which typically delegates to HTML syntax highlighters. In order to update Ember's syntax highlighting, we'd have to do one of these per highlighter:
All of these represent a significant amount of effort. Finally, we'd need to get editors to avoid treating For these reasons, and especially because we're talking about a temporary stopgap, I really prefer that we go with |
I agree with both of these points. Since you already have to learn the Also, I agree that At the end of the day, just like the |
Completely agree. We need to have some solution in the short term.
Also agree 😄 My concern with this proposal then is not so much focused around the particular sigils ( I suppose in some sense this is really a question of how far out template imports are. If it's a month or two (not suggesting it is, but just to demonstrate the point), then clearly the long term cost of this RFC is not worth the benefits for a couple months. If it's a couple years, then having a "blessed" interim solution feels appropriate. If it's somewhere in the middle - it gets a bit fuzzier? 🤷♂️ I realize accurately predicting this is a fools errand, but I want to call attention to the tradeoff at least, in case anyone more informed than I could weigh in. |
@davewasmer Totally agreed with you on the tradeoff. Speaking personally, if the answer is not "in less than a month," I think we have to plan as though it's far out, as past history suggests we often get these kinds of guesses wrong. I think our motivation is also to not plan based on having a better new design "soon," which creates pressure to ship something fast even when if we discover flaws. I think in this case, even if we do end up shipping template imports in a couple months (which I'd love to see but honestly think is a stretch), this proposal is a relatively small extension on top of the existing system that will need to be migrated from anyway. So in the grand scheme of things, I think any additional "churn" cost this introduces will be a drop in the bucket compared to the overall migration from runtime lookup to template imports, which we'll need to heavily automate anyway due to the scope of the change. |
Thank you to everyone who has provided feedback so far! We discussed this RFC at the core team meeting today and reached consensus that my claims about relative difficulty of Given core team consensus, we are moving this RFC to Final Comment Period to ensure people have an opportunity to raise any additional concerns or provide additional feedback before we merge. |
|
||
Finally, the goal of this RFC is to make it possible to recommend that users always use angle-bracket invocation for components other than control flow (`if`, `each`). | ||
|
||
This means that we should update the syntax conversion guide to no longer say that `{{` syntax is sometimes required, and avoid recommending it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means that we should update the syntax conversion guide to no longer say that `{{` syntax is sometimes required, and avoid recommending it. | |
This means that we should update the [syntax conversion guide](https://guides.emberjs.com/release/reference/syntax-conversion-guide/#toc_when-to-use-classic-invocation-syntax) to no longer say that `{{` syntax is sometimes required, and avoid recommending it. |
|
||
![rental-listing in the docs](../images/457-dasherization.jpg) | ||
|
||
After this RFC, the documentation should add a "Zoey says" sidebar that describes the rule in more detail, and mentions that you can refer to components nested in a directory with the `::` separator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After this RFC, the documentation should add a "Zoey says" sidebar that describes the rule in more detail, and mentions that you can refer to components nested in a directory with the `::` separator. | |
As part of shipping the feature in Ember.js, updates should be made to the API documentation for [components](https://emberjs.com/api/ember/release/classes/Component). Then, the Guides section on components should add a "Zoey says" callout that describes Angle Bracket normalization rules. The callout can mention that components are nested in a directory with the `::` separator, and it will have a link to more details in the API docs. |
I want to pump the breaks a bit here and this is going to be unpopular, but I am in favor of the proposed alternative to recommend the usage of
Introducing new syntax seems like a fairly strong signal to send for something that will, from my read, eventually become a candidate for a deprecation cycle.
Agree and for my part, if we must introduce a new syntax I do prefer |
@jrowlingson as was mentioned later down in the thread, it appears the core team members don’t see this as a “stop gap” or “interim” feature that already has a planned deprecation/end of life before it even ships. Looks like this is intended to be a first class pattern that’s supported for a long time to come, even after when/if template imports do land. Imo, trying to teach something like the ‘{{let}}’ pattern as a “for now, wait and see” solution is a much stronger signal to send (and is quite an ugly/awkward workaround I think), especially considering it’s only needed in certain situations, and since this is something that was already supported via the old curly syntax. As we’ve seen with Controllers/routable components etc in the past, designing current patterns around a possible/eventual future pattern is generally not a good idea. :-D |
@billdami That is fair - I may be underestimating the teaching cost of the |
I agree with @billdami that the high-order-bit here is the understanding cost. People encounter the need for component grouping earlier in their Ember experience than the point where they understand |
Since MU is on hold, I'll be sticking with pods going forward. Will this work with the pods structure? |
The invocation proposed in this RFC will work the same in "pods" and "classic" resolver setups. |
Does it make sense to restrict depth of nesting? <Namespace::ComponentName::IamLikely::APrivateComponent>
<Content />
</Namespace::ComponentName::IamLikely::APrivateComponent> I don't like an idea to write a closing tag for deeply nested components. For deep components, it's more natural to use Assuming we have an inline form(inspired by ember-let) of {{let Short=(component "nested/path")}}
<Short>
<Content />
</Short> |
@ro0gr for reference this RFC doesn't stop you from using I think restricting nesting depth could be a linting preference since it would likely vary from team to team. |
We decided to accept this RFC at the framework core team meeting today. Thanks @wycats for working on this and everyone else for the discussion 🎉 |
Since Ember has decided to repurpose the `::` sigil to mean "nested folder invocation" (see emberjs/rfcs#457), this introduces `$` as a replacement sigil. Ultimately, usage of `::` will be deprecated by this addon (though that will be in a future commit).
Since Ember has decided to repurpose the `::` sigil to mean "nested folder invocation" (see emberjs/rfcs#457), this introduces `$` as a replacement sigil. Ultimately, usage of `::` will be deprecated by this addon (though that will be in a future commit).
Since Ember has decided to repurpose the `::` sigil to mean "nested folder invocation" (see emberjs/rfcs#457), this introduces `$` as a replacement sigil. Ultimately, usage of `::` will be deprecated by this addon (though that will be in a future commit).
Rendered