-
-
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
Angle Bracket Invocations For Built-in Components #459
Conversation
56aebd2
to
d2434db
Compare
d2434db
to
2216eec
Compare
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.
👏 I like it, thank you for putting the plan together @chancancode!
Co-Authored-By: chancancode <[email protected]>
Co-Authored-By: chancancode <[email protected]>
|
||
...becomes... | ||
|
||
<Input @type="checkbox" @name="email-opt-in" @checked={{this.model.emailPreference}} /> |
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.
If we were building these components from first principals, would we choose this API? Might this be an opportunity to build something better, removing type
and the overloaded input API and instead provide dedicated components such as <TextInput />
and <Checkbox />
? (with possibly better names)
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.
I think there are advantages sticking close to underlying HTML element that it is trying to bridge (even though we don't fully support all the input
types too). Also, this RFC just tries to fix a narrow problem (provide a 1:1 transition path from the curly invocations) without opening the can of worms of "hey.. what if we also just fix this one thing......", they can be explored in separate RFCs IMO, so long as they are codemod-able I don't think there is a lot of extra cost to do them later.
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.
👍 thanks, agreed. Having the opportunity to codemod our way to some possible future API means that this is a nice incremental improvement
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.
If we were building these components from first principals, would we choose this API?
No, but it is not this RFCs intent to "fix the design of the underlying components", that should be done separately.
tldr; what Godfrey said 😸
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 is the same point I came here to make. In particular, given ...attributes
and the semantics of type
vs. @type
on a regular <input>
, it seems like it might be worth deprecating {{input}}
(along the lines proposed here) and using the internal mechanics which create the correct input type to offer good suggestions. "You attempted to create a checkbox with {{input}}
; we recommend <CheckBox />
..." etc.
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.
I think both could work as @rwjblue points out this RFC is to bridge the gap with the commonly used components today. For instance if a Ember dev reads the guide materials on curly vs angle bracket and sees {{input type="number"}}
being used it's a much smoother path to change it to <Input @type="number">
vs having to know <TextInput type="number" />
or all of the different variations.
A more angle bracket 1st set of input components would be great! We could even use codemods or build time AST transforms to rewrite to these new components in the future. But, for now let's make a way to use the built-in components we have.
Wasn't recommended to pass an id to |
By migrating to the angle bracket invocation does that break the two-way binding that the |
@ddoria921 it does not, angle bracket invocations does not behave any differently there |
I would suggest to re-think the proposed deprecations on {{#link-to 'help'}}Help{{/link-to}} And I see a deprecation along the line {{#link-to route='help'}}Help{{/link-to}} Then, I'll still have to refactor that again later to angle bracket invocation. I believe this would introduce a lot of churn, as we use Instead, I think it is better to eventually deprecate the whole curly brace invocation form, and directly suggest the use of a codemod. |
One of the best things we were doing, was to avoid My concerns are, that until we have imports to specify which Here is an OSS example of mine: https://github.com/gossi/spomoda/tree/master/client/src/ui/components/input My hypothesis is, there is a lot of private implementations as well, that said, the number of those is unknown. Are my concerns real? Like would an |
The input component in your app would “win”, and thus this change should not cause any issues. |
Hmm, yes I see what you mean. I think this logic does make sense. What do you think @chancancode? |
@chancancode So to clarify, angle bracket components will only have two-way binding in these few scenarios? If that is the case, are we worried about inconsistency of the API? |
@crhayes I was confused about this too, but I think I understand the way it works now. All ember components have two-way binding and all glimmer components will have one-way binding. I think it starts getting confusing because both can be used with angle bracket invocation. Sounds like these built-in components will be ember components to maintain backwards compatibility. And I can be totally wrong about this so @chancancode can chime in and correct me if I am 😅 |
@ddoria921 @crhayes That's pretty much correct. The rendering engine exposes the low-level capability to do "two way bindings", However, all of these are API-design decisions/implementation details of the components being invoked, and not related to how you invoke them. The curly/angle bracket invocation style is purely a syntatical difference on the caller side, and the component doesn't care or know about which one you used (other inferring it from the fact that you can't pass positional arguments with angle bracket, so if you did, you must have used curlies). |
@gossi even though it may work, I would strongly suggest that you (and other addons) rename them into something else to avoid confusion and other tooling issues (codemods, template lints, etc). Typically, This RFC is really just fixing that inconsistency, and I think it's quite possible that after things are re-implemented "correctly", your component will also override |
@mydea @rwjblue I do think that running the codemod and using angle brackets is the correct way to resolve those deprecations. The curly invocation is only "kept around" in the sense that all components can be invoked in either style (curlies or angle bracket). It would be inconsistent for the built-in components to behave differently on that front. Just because you can doesn't mean you should though, and I think with Octane, the idea is to push the whole community to use angle bracket exclusively for all components. The only exception would be for control-flow-like "components", such as I agree the deprecation messages should just suggest the angle bracket invocation style. I do think In terms of a rollout plan, ideally I think everyone should just run the codemod as soon as they upgrade, so none of these should matter. I don't know if that's what people do in practice. If it causes problem, I suppose we can rollout the deprecation a few releases later, after people had the chance to run the codemod. I'm not sure if this is good or necessary though. I would prefer that people can run the codemod on the whole app, and get immediate negative feedback when they used the old style in new code out of habit. Maybe this wouldn't work because old addons will cause too much deprecation noise? (Do addons really use links though?) |
Would this satisfy your concern if the deprecation messages looks something
|
@chancancode Thanks for the response. Please correct me if any of the following is inaccurate. Although angle brackets can be used to instantiate both ember and glimmer components, in the future I imagine angle brackets will become synonymous with glimmer components. In a world where all (or 99.9%) of components are glimmer components invoked with angle bracket syntax, and args are bound one-way, it would be unintuitive for these input components to continue leveraging two-way binding. Is that a reasonable concern? |
Personally, I think it would not be ideal to update e.g. from Ember 3.10 to 3.11 (as an example), and suddenly getting 300 deprecation warnings (as we have about 300 We aren't quite ready yet to go all-in on AngleBracket invocation (mainly blocked by #457, so hopefully not for long ;) ), and we'd like to do the migration in our own time/on our own terms. (We have never used the inline form, so I have no opinion about that) |
Hmm, I'm still not convinced. Most of the things, that the original helpers were for are nowadays all achieveable with modifiers, e.g. ember-on-modifier for the inputs and there were/are a couple of RFCs for Second is that Also, I can see a need to update legacy stuff to nowadays API. As a maybe good compromise, those 3 things can be moved into an addon, that will be installed as part of the official blueprints (until a certain version?). Existing helpers will throw a deprecation message with the hint to the addon (if it's not installed). That would mean, people have a change to opt-out 🙈 |
@gossi @michaelrkn As mentioned in the motivation section, this RFC is not really about fixing/changing the existing APIs. I think it would be fine to open a separate RFC to discuss adding more features, fixing bad APIs moving these into an addon, deprecating them, etc. The main problem we are trying to solve is that:
That leaves us with three problems:
1 is a bit embarrassing, but 2 and 3 on the list are especially bad because they break the mental model. Personally, I would just consider them a BUGFIX given the angle bracket syntax is supposed to be more or less a syntactic sugar of the curly invocation style (like the difference between This RFC tries to propose a very targeted fix to these inconsistencies in current versions of Ember (which is the goal of Octane: a coherence checkpoint). It does not preclude making any further changes to these APIs. |
@crhayes I don't think that is quite right. Angle vs curly are just two invocation syntaxes that ultimately said nothing about the thing you are invoking. For example, these are different but equivalent invocation syntaxes in JavaScript:
However, just from this, there is no way to know what Components are the same, it is not possible to tell how a component is implemented from the calling syntax. I think for the foreseeable future, there will be a mix of Sometimes, some of these components (especially internal ones, but also custom components) may leverage unusual capabilities. In the JavaScript analogy, generally functions can't "block", but in the browser, functions like I personally don't think two-way bindings for this use case is actually Bad™, albeit a little unusual. But we could also have the conversation of redesigning these APIs to do something else, but as per above, that's a different RFC. |
Changes based on feedback from framework core team meeting today:
|
@chancancode I was just thinking in the future glimmer components would be preferred over ember components, and so for new people coming to the framework their mental model of components will be that of glimmer backed angle bracket components. These components will have one-way binding semantics. When they see |
Thanks for summarizing our feedback from the meeting today @chancancode, and folding it in on that last commit. The Ember.js core team has agreed to move this into Final Comment Period with those changes. This is a quick moving RFC, but as it basically suggests making currently available components invokable with |
@crhayes I think that you're right that in some future, lots of Ember developers will learn the Glimmer Component API first, and come to form intuitions about the behavior of components from that experience. (aside: with enough time, I don't think there will be a big difference between angle brackets and curlies from this perspective -- people's impression of components will match what they learned: the behavior of the Glimmer component base class). Between now and then, we will need to come up with a plan, as a community, for what people should do about situations that feel like a good fit for two-way bindings. Maybe the solution will be to double-down on DDAU, but as @rtablada said, that can get fairly verbose. Maybe we'll try something like a better version of Either way, we will need to find a long-term solution for how to think about "data-up" situations, probably soon. In the meantime, we'd like to be able to recommend that people use angle-bracket syntax for components in general, and avoid unnecessary caveats. #457 is about eliminating one of those caveats (directory nesting) and this RFC is about eliminating another (built-in components). |
Sweet, thanks for the insight. Agreed on removing impediments to using angle bracket components... I personally can't wait! |
Just wanted to note my above comment was one of those "GitHub didn't tell me there were more comments"; I have no objections given the additional clarification that was provided between when I started the comment and when I posted it. I'm 👍 on this as it stands. |
Will passing attributes work as expected? So can I do Another thing: While I totally understand that waiting for the perfect solution leaded to problematic situations (controllers) do we have any idea how we can ever get rid of the two way binding of Thats exactly why I'm asking for |
@luxferresum in your desired scenario, is there a reason not to simply use plain-old |
I use 100% native ConfusionMy primary concern is confusion between This leaves it open to what should happen when the user does It seems this RFC leaves the whole attributes story out! This means undefined behaviour and this is very bad. So I ask for clarification. This RFC could define that passing attributes to Now assuming we support And what if a user then does If we just ignore all attributes and don't throw then
Here the And if this is only by undefined behaviour and we ever decide to support passing attributes this could break a lot of code. The future of
|
I was also concerned about the mental model breakage, so this explanation should go into the RFC. Basically, the BUGFIX is also introducing tech debt. Shouldn't this be addressed in the RFC, too? Sth: There will be a future RFC coming, that will handle this, e.g. a possible solution would be to move those into a legacy-compatibility-addon? |
I made <Link
@route="some.route"
@models={{array 123}}
@query={{hash foo="bar"}}
as |l|>
<a
href={{l.href}}
class={{if l.isActive "is-active"}}
{{on "click" l.transitionTo}}
>
Click me
</a>
</Link> |
Co-Authored-By: chancancode <[email protected]>
@luxferresum More to the point: this is not really a question about Generally, any component is allowed (and broadly speaking, encouraged) to forward attributes. Being able to add classes to the output of any components with a consistent API is one of the biggest benefit of this API (again, see #435). As for passing the First of all, despite what it looks like, this really isn't "setting an attribute" (it wouldn't make sense to set an HTML "attribute" to anything other than a string). For historical reasons, it will set the Second, the semantics of this is no more "undefined" than using the However, it's indeed the case that if you do this, you are likely interfering with the component's inner working which would make it a bad idea. As we discussed on RFC #435, the fact that you can do this at all certainly makes it a sharp tool. On the other hand, it's not that different from adding a wrapper around the component and capturing the event there, or rendering an element inside a In general, when you assign an event handler to a component using splattributes, you can't predict the exact behavior that will result, because you're interfering with the component's event handling. In this case, the input component never specified what events it relies on, so interfering with any of them could be problematic. Personally, I think this is not any more of a risk than, say, passing
Again, this is true for angle bracket components in general, and I suspect it's just a matter of getting used to the syntax. When we originally considered this syntax, we felt that the experience from React developers shows that people can differentiate easily between components starting with capital letters and HTML elements starting with lowercase letters. When we made this decision, we also felt that proper syntax highlighting would further reduce the risk of confusion.
It would.
You should use
It seems like you are unhappy with the current APIs of the This RFC was narrowly scoped to bring the existing |
When I said "mental model breakage", I was referring to model proposed in the accepted RFC #311. Namely – angle bracket vs curly braces is purely a syntactic thing, and works across the board for all components, regardless of their implementation ( Specifically, we explicitly decided not to do what was proposed in a very very early version of the proposal: making angle brackets syntax the opt-in for one-way bindings. That plan was abandoned a very long time ago. Instead, we decided to make angle brackets semantically equivalent to curly syntax to facilitate migration. Those early design ideas may be where some of these confusions are coming from?
I don't see what technical debt this is incurring. Can you elaborate?
I personally haven't formed an opinion around that, and I don't think it would be appropriate for this RFC to speculate about that. Instead, I tried to focus on what we need to do to allow existing built-in components to work correctly. |
We decided to accept this RFC at the framework core team meeting today. Thanks @chancancode for working on this and everyone else for the discussion 🎉 |
Rendered