-
-
Notifications
You must be signed in to change notification settings - Fork 407
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
An ability to specify default values for @named arguments #479
Comments
To be clear, I do NOT want to be using I want a way to provide default values for named args and keep using them as This is true both for Classic and Glimmer components. |
Update (2021-05-23) (and after having spent a lot more time with Octane, and teaching it to folks) I no longer think the opposite of @rwjblue
|
One thing to consider for a design around this is that one of the points of arguments is that they're immutable; so you'd want an approach—whether using decorators or something else—that lets that guarantee still be upheld for the idea of defaults. Personally, I strongly prefer the explicitness of using a getter and saying "This is my own derived state," however it is derived. That makes it clear in the template that when I see That said, optional arguments with defaults in JS are a thing, so I'm not opposed to coming up with a comparable solution here; I just want it to be the case that we don't lose the other benefits of named args. |
I do not want to access some of my arguments via Also, default values for arguments are a very common use case. It's not some caprice that developers, be they quirky enough to desire it, should figure out on their own. The worst is that when providing a default value with a getter but not passing any value from the parent template,
How is this ☝️ better than these 👇 ? export default class MyComponent extends Component {
foo = 'a default value';
get bar() {
return this.attrs.bar || 'another default value';
}
} Either way you still need to revert to
But arguments passed externally do get written into the And I want to avoid mistakes and confusion, my only option is to always use But I don't want to be a black sheep. And I do want to see a distinction between arguments and properties in my templates. It's just arguments with default values are still arguments! They've always been. |
Note that there's no linter (and can hardly imagine one to exist) for warning you that you're using |
I think maybe we have something else to talk about now <3 Can you give an example of something where'd you have this many args?
fwiw, the point is not to remember, but to be able to discover. Apps are too big to keep in your head, ya know?
imo, the getter is better, because there is no string access on args.
that defeats the purpose of read-only args. ya don't always need a default. :-\ |
Oh wow, doing |
That's what I said in the top post, and this is exactly the reason to request an RFC for default values. |
that's probably because we have to support classic accessing for those not fully on the named args train. once we get rid of classic arg accessing, that won't be an issue |
I can agree with that for Glimmer components. They are more strict and are a new entity, so a shift of a mental model is acceptable. But everyone's mental model for Classic components is that an argument can be accessed in a template the same way, regardless of whether it has a default value or not. Adding (or removing, for that matter!) a default value to/from an existing argument did not force me to update my template. It just worked, and that's how it should be. The named arguments RFC changed that. 😒 But note how adding a default value now requires rewriting a template from It is an inconsistency and a source of confusion. I've been always subconsciously uncomfortable using |
This was discussed elsewhere (apparently not on the RFC) and I seem to recall the answer (for the time being) was to be |
you could still use classic / ambiguous access: where, import { Component } from '@ember/component';
export default class MyComponent extends Component.extend({
foo: 'default value',
}) {
// class body
} |
There are two huge issues with this approach:
|
Which equally defeats the purpose of named arguments RFC. BTW, you don't need to use |
|
And restaurants are not a viable solution for people with food allergies. Is is a reason to ban restaurants that use fish, garlic, honey, dairy products, etc? Note that having a meaningful way of defining default values for arguments does not prevent you from using |
imo, named args is more of a single piece towards getting to the Octane mental model -- it's not meant to be used with the old mental model where you can have mutable args.
🤷♂️ |
Sorry, I'm not following this reference. I just wanted to provide a solution that will work in all scenarios.
The new Glimmer Components purposefully minimized the number of lifecycle hooks and such to reduce property name collisions, so IMO I'd rather avoid adding more things back in. Again, I was trying to highlight a solution that allows you to put default values in your component class, or just put them in your template. The developer gets to decide. |
If Classic components are now considered to be transitional, and full migration to Glimmer components will be encouraged, then I agree that this is reasonable. But I thought Classic components would not be deprecated? |
we'd need another RFC to deprecate anything in classic ember. I don't see that happening for a while though. |
This is the long term goal, absolutely. We haven't fully deprecated classic components for a few reasons:
I can confirm that named arguments were not ever meant to work particularly well with classic components. The fact is that they were designed with Glimmer components in mind. Like @chriskrycho pointed out, there's a strong argument that including some API for setting them may muddy the waters a bit and make it harder to reason about a system that is now very pure in Glimmer components - My personal strawman would be a second export, separate from both the class and the template, something like: export const defaultArgs = {
foo: 'bar',
baz: () => [],
} Note that initializer functions would be allowed so users could provide defaults for values like objects and arrays. This gets a bit tricky though, as it means we need to define a few things:
Another possible option would be a export function defaultArgs() {
return {
foo: 'bar',
baz: [],
};
} I imagine this could get expensive if used incorrectly though |
@pzuraq, with Classic Ember, one could set up a computed property and get it overridden from a parent template: Component.extend({
name: null,
slug: computed('name', function() {
return snakeCase(this.name);
})
})
I know that overwritable CPs are out of favor these days, but that technique was very simple and efficient. Modern alternatives require two distinct properties: slug-from-parent and slug-default, leaving a chance of mistake without the code structure providing clues about what's wrong and what the right usage should be. @pzuraq, does your second export suggestion support referencing other arguments and properties like this? |
Personally I would want to avoid this. Allowing default values to reference incoming arguments or component state means we will see a split between business logic in component classes and the defaultArgs values, and it’ll make things harder to follow. If you need logic like this, I would use a getter instead. We could of course also add the ability to reference incoming args later on, if we find it would be valuable. What I would propose would be keeping it minimal - just plain primitive values - for the first iteration. I don’t think we should ever add the ability to reference component state. |
Before the named components RFC, this served as self-documentation for component arguments contract: Component.extend({
name: undefined,
role: 'user',
}) Now not only providing argument defaults is impossible, but even documentation no longer makes sense, since Even if y'all refuse to implement a reasonable way to specify argument defaults, there must still be some guidance! An officially recommended way of self-documenting component arguments and providing default values. Is must be in the guides, mustn't it? |
This kiiinda sounds like a similar role typescript would provide interface IArgs {
name?: string
role: string
}
export default class MyComponent extends Component<IArgs> {
} you then get earlier warnings/errors when misused! :)
I think we need an RFC, as we'd need a change to the component API. I think something like this (inspired from React's prop-types system) is our best option (imo) export default class MyComponent extends Component {
static defaultArgs = {
name: undefined,
role: 'user',
}
} and things are still accessed via thoughts on that? |
I don't think it's a good idea to place the default value directly on the class, because it'll force users to make a class and incur more overhead even if they want to make a template-only component (which I also see being more common in the future, especially with importable helpers and modifiers). I still think a separate named export makes the most sense, as it works whether or not there is a class for the component. As for documentation, I think attempting to document components like classes is the wrong approach. In ember-cli-addon-docs, we specifically made component documentation document what matters to components - their public API, arguments and yields. All class fields and methods were considered private, internal implementation details: We achieved this by layering a post-processing step on top of documentation tools, such as ESDoc and YUIDoc. Both these documenters, and most others, output to JSON optionally, so the post-processing is relatively lightweight. Future documentation tools that are in the works (such as code-to-json) are focused on this use case primarily, because it's so flexible and allows this type of customization. This strategy currently requires users to annotate each class field that they intend to be an argument (you annotate the entire class with the values yielded). Both @NullVoxPopuli's idea and my own would be much easier to automate, using TypeScript's type inference to detect the value's types (code-to-json uses TS even for normal JS) and falling back to annotations when required. I think they would be equivalent to automate given this, so I would prefer using a separate export for documentation. I spent a lot of time thinking about how to make documentation better during my work on ec-addon-docs, so I'm definitely very aware of these concerns, and it's a very high priority to me to have a fully automated, minimal, maintainable solution 😄 |
Note that there was some discussion about default args on the Glimmer components RFC: #416 One point that came up was not tying this to a component class, so that we were able to use default arg values on template-only components. |
What if default args were defined in the template? |
I thought that would be ideal but thinking more about it, either way will feel somewhat disconnected from each other. For example, would somehow putting defaults in the template effect |
It should be possible to define TypeScript types! The rest of the app should know which type each argument property has. |
TS types already exist for Glimmer components; but importantly no design in this space can require an app to use TypeScript. More, TypeScript does not supply any run-time data, so it is not a useful bit for supplying default args. |
@chriskrycho Wat? I never said I want to enforce TypeScript for everyone and I never wanted to extract run-time data from TypeScript. Was that a straw man attack 😬 ? When I design a component, I want to define a type for each argument property -- in such a way that other portions of the app recognize those types and provide warnings when types don't match. Obviously, I want to do it in the same place in code where I specify default arguments. Previously, this was working perfectly (at least for Classic components): class MyComponent extends Component {
name!: string; // required argument
prefix?: string; // optional argument
role = 'user'; // optional argument with a default value
} This approach was serving three tightly interconnected purposes:
The named arguments RFC broke all three. |
What is the official way of defining types for Glimmer component arguments? |
@lolmaus I was just clarifying for everyone reading the thread – no attack implied. 😄 In TS, your example would look like this: interface Args {
name: string;
prefix?: string;
role?: string;
}
class MyComponent extends Component<Args> {
get role(): string {
return this.args.role || 'user';
}
} I also disagree with your assertion that "the named arguments RFC broke all three" of these; what it did is separate them from the internal state of the component by moving them into their own namespace. I totally get that it's a big change, though! |
@chriskrycho Ok, so I have the following issues with this approach.
|
@lolmaus I believe everyone understands your complaints at this point, as you've reiterated them several times now. I understand why you feel the way you do, but I disagree; I think the tradeoffs are worth it (having used variants on them for quite some time). 🤷♂ If you have a proposal, I think everyone would love to hear it, but repeating the same complaints isn't changing anything here. |
I've been reiterating on my complaints in a hope to achieve agreement that the issue is legit.
I've been thinking of employing decorators: class MyComponent extends Component{
@arg name!: string;
@arg prefix?: string;
@arg role = 'user';
// Alternative:
@arg('user') role: string;
} Not sure how to do it for Glimmer components, though. :( Maybe a class decorator? interface Args {
name: string;
prefix?: string;
role?: string;
}
@args({
role: 'user'
})
class MyComponent extends Component<Args> {
} |
@lolmaus we absolutely think your concerns are legit. For context, when originally designing namespaced args and Glimmer components, this issue did come up, and we discussed it in depth quite a few times. In all those discussions, we kept coming back to the many issues with the way that arguments work in classic components, and decided that the pros of namespaced arguments and separation of concerns outweighed the cons. The explicitness may feel verbose, but the extra safety it provides is generally worth it. This is similar to our current stance on Mixins. That said, we also intentionally left room for future solutions here, such as a separate
I realize that not having this ability right now is frustrating, but I think this is also part of the philosophy Ember is taking on right now - iterative steps and improvements, day over day and year over year, rather than complete rewrites that attempt to solve every problem all at once. There are plenty more features, ideas, and improvements we have in the wings for the post-Octane world, and we'll keep on shipping them one at a time 😄 Discussions like this help us prioritize and gather ideas for how to solve the problems better, so we absolutely appreciate you bringing this up, I just want to reiterate, we get where you're coming from, and we're taking it seriously. We just need to do the design work, and iterate toward a solid solution. |
I'm late to the party. I found this discussion somewhere left it open in a tab and now I had the time to read through it, totally worth it. However, all along the way, you need to do a mindshift - that is particularly not easy when trying to bake old patterns into a new system (which easily become anti-patterns as in this case). So some things I learned, that I hope that help you (since not everything is finalized, those things may change):
interface MyArgs {
foo?: string;
}
export default class MyComponent extends Component<MyArgs> {
foo: string;
constructor(...) {
this.foo = this.args.foo || 'default goes here';
}
}
The best tip here (and I gonna repeat myself for this): It's a mindshift, embrace it. Throw your "old habited" balast over board and come into a clean, explicit and more structured ember-world :) //cc @locks @jenweber -> is that something for the guides, too? Regarding the migration part? |
I've been thinking about this more, along with the ability to specify some sort of I think it may actually make sense to use static class properties instead. It could look something like: export class MyClassBasedComponent extends Component {
static defaultArgs = {};
}
export const MyTemplateOnlyComponent = <template></template>;
MyTemplateOnlyComponent.defaultArgs = {}; And the same could be done for some sort of ArgTypes system. The other option would be to have a different syntax specifically for TO-components, but I don't think that would be ideal. It'd make converting from a TO-component to a class-component much harder, and would make it harder to statically analyze. |
I found reading through the comments for this quite frustrating because it seems I hold very different things important than most of the people here. That's going to make the rest of this post feel somewhat rant-y, which I apologize in advance for. The named arguments RFC broke an extremely basic and commonly feature of components, which are the modern building block of front-end UI. The fact that it was accepted and implemented without any support for default values (all the current solutions are extremely bug-prone because anyone using The two main considerations being expressed in this thread preventing a solution being agreed upon (many of the decorator-based solutions have seemed great to me) are things that I utterly disagree with. They are:
It's hard to participate in a debate like this where the disagreements seem so fundamental, so thanks for bearing with me. |
@maxbogue I understand the frustration. The core team has been in a holding pattern recently as we put the final touches on Octane, so we haven't been actively working on or reviewing RFCs, which is why progress has been slow. I don't think we're as philosophically disconnected as it may seem though. For your first point, it's important to realize that you do get a clear distinction in a function definition as to what is an argument, and what is a local value. function myFunction(arg = 'foo') {
let localValue = 'bar';
} What Ember did historically would be, more or less, translated to JS (not in a literal way, but in a semantically similar hypothetical) like this: function myFunction() {
this.arg = this.arg || 'foo';
this.localValue = 'bar';
} Personally I think this is much more difficult to reason about, and I think this is why a lot of folks both in the community and on the core team want to make sure we don't reintroduce semantics like this. This doesn't mean that everyone is against defaults in general. JS also didn't have default values for arguments for quite some time, but they have definitely been valuable since they were introduced, and I think that we can probably find a solution that works for all of our use cases, and still maintains the semantics we worked for with named arguments. Re: prioritizing template-only components (something I've mentioned a few times in this thread), IMO it's is not prioritizing them over class based components, but making sure we don't design ourselves into a corner. If we were to come up with a design for defaults that, for instance, limited our options for designing template-imports, that would not be ideal. If we were to come up with a design that had no way of translating into TO components, that would also not be ideal. It's mostly about doing our due-diligence, and making sure we've considered as much as possible in the design phase. I'm hopeful that we'll be able to make progress on this soon! And again, I understand the frustration and hope the explanation helps a bit. Thanks for your feedback! |
I like
I guess it's pretty unclear to me how you would ever do something like this with TO components, because templates are just templates and the JS is everything else about a component. This is pretty clearly part of the everything else. Trying to make every component feature going forward TO-compatible seems pretty problematic to me. |
So, for some extra context here, one thing that is being discussed a lot is making TO components much more viable for use in the majority of component situations. Today they're fairly limited in what they can do, but with template imports it may be possible to make them much more powerful. import PowerSelect from 'ember-power-select';
function getCityNames(cities) {
return cities.map(c => c.name);
}
function getCountryNames(cities) {
let countries = new Set();
cities.forEach((city) => countries.add(city.country));
return Array.from(countries);
}
export default hbs`
City:
<PowerSelect
@selected={{@selectedCity}}
@options={{getCityNames @cities}}
@onChange={{@onCityChange}}
/>
Country:
<PowerSelect
@selected={{@selectedCountry}}
@options={{getCountryNames @cities}}
@onChange={{@onCountryChange}}
/>
`; Or a frontmatter version: ---
import PowerSelect from 'ember-power-select';
function getCityNames(cities) {
return cities.map(c => c.name);
}
function getCountryNames(cities) {
let countries = new Set();
cities.forEach((city) => countries.add(city.country));
return Array.from(countries);
}
---
City:
<PowerSelect
@selected={{@selectedCity}}
@options={{getCityNames @cities}}
@onChange={{@onCityChange}}
/>
Country:
<PowerSelect
@selected={{@selectedCountry}}
@options={{getCountryNames @cities}}
@onChange={{@onCountryChange}}
/> With this type of a setup, it would be possible to use TO components for many more use cases, especially when no root state exists on the component. Helper functions can take the place of computed properties/getters, and you could define modifiers inline as well potentially. Today, those would all have to be global, and the cost to setting them up is pretty prohibitive because of that. This isn't all set in stone at all, template imports could also end up being much more minimal (e.g. only allow |
Got to be honest, I'm not a fan of putting JavaScript in frontmatter, that belongs in the |
There are a few different general directions that are being discussed, some that are even more minimal (no JS except import statements allowed). I posted two of the options mainly to demonstrate why some folks think TO components are worth investing into, myself included, and because I don't want to give the impression that anything has been decided - still plenty of RFCs to go before we finalize the syntax! |
@pzuraq This is a bit of a side tangent, but what’s the main driving force behind having that TO component you showed (which wasn’t just a template, but verbiage aside) over a standard/glimmer/single file/class component? I get the explicit import, but the rest just seems like needless confusion, and could be easily done with a template and a class rather then template and functions. Is it performance? Memory usage? “Ergonomics”? If there’s a better place for this question, point me there and i’ll read ask their. |
@webark let's continue this discussion on Discuss, started a thread over there |
I'm closing this due to inactivity. This doesn't mean that the idea presented here is invalid, but that, unfortunately, nobody has taken the effort to spearhead it and bring it to completion. Please feel free to advocate for it if you believe that this is still worth pursuing. Thanks! |
I think someone had an idea that layered on top of: #779 (and #800 / #748 ) interface Signature {
Args: {
foo?: string;
}
}
export const Foo: TOC<Signature> = <template @foo="default value">
always shows something: {{@foo}}
</template> tho, maybe |
Previously, I was able to provide default values to arguments like this:
component.js:
template.hbs:
result:
It was a simple, efficient and very declarative way of specifying default values. It both worked well and was obvious to developers reading the code.
With named args, this no longer works. Though an argument passed externally does overwrite
this.foo
, setting an initial value tothis.foo
does not affect@foo
in the template.As a result, there is no way of specifying a default values for arguments!
The only workaround I'm aware of is to keep using
{{foo}}
or{{this.foo}}
instead of{{@foo}}
. This works, but it defeats the purpose of @-named args in templates.Thus, we need a simple, declarative way of specifying default values for arguments used in components.
A developer should be able to tell which arguments are gonna have which default values by simply looking at the component JS file. Those definitions should not be obscured inside a hook such as
init
.The text was updated successfully, but these errors were encountered: