Element Modifiers

[SergeAstapov]

Rendered

---

This RFC supersedes the original RFC #353 "Modifiers"

[chriskrycho]

Status: this was on the agenda for this week’s Framework Core Team meeting, but we had a couple of longer discussions about other open RFCs before we got to it. I expect we’ll be able to cover it next week.

[chriskrycho]

Some notes on last week’s Framework team meeting and one-off discussions—

1. We should tweak this to highlight the design motivation as such. This will be a couple small tweaks to the text, which I’ll suggest in a review today or tomorrow. Compliments to @SergeAstapov, though, who did a great job spelling out the motivation in those terms already: it means the tweaks here are small and minimal. This distinction is important because we are not establishing a norm of just adding popular packages to the default blueprint—we are adding this one specifically because it addresses a fundamental gap in the story for authoring Octane apps, and because it already has a well-rationalized API matching the framework.

2. We do want to go ahead and just add ember-modifier to the default blueprint. This is explicitly and intentionally adding some incoherence to Ember as we begin pushing toward Polaris, where we expect to resolve that incoherence. Specifically, we will now have imports for both @ember/modifier and ember-modifier available, and folks will encounter that regularly as they start adopting strict mode via First-Class Component Templates and need to import { on } from '@ember/modifier';. If they happen to also define a modifier locally, that can look a bit strange (I actually call this out explicitly in my EmberConf talk, as you’ll see next week.)

(2) is related why we’re not just taking the path @Windvis suggested (though that is also a space we want to iterate on): we see a path to rationalizing modifiers, effects (of which modifiers are a special case), helpers, resources, etc. into a single unified design. Continuing to ship ember-modifier allows us to separately ship @glimmer/modifier in the future (per @rwjblue’s suggestion) building on that unification, or to adopt the existing ember-modifier design there and eventually make a breaking change, or other variations in the space.

The key is that we have a minimal and incremental path forward by adopting ember-modifier:

• there is zero churn for existing users: they just keep using the same library
• we can move to either of @ember/modifier or @glimmer/modifier at will in the future, with a full set of options for exactly what we move there at that time
• we can also tackle the question of function-based modifiers a la #757 separately, and that can inform what we do or don't add to @glimmer/modifier etc.

I'll review shortly with the recommended tweaks, and we’ll hopefully talk about it again at this week’s Framework meeting!

[rwjblue]

We do want to go ahead and just add ember-modifier to the default blueprint.

Hmm, this seems somewhat strange to me (and I missed the core team meeting referenced). Can you explain why? Is your expectation that every app will create at least one modifier of it's own (that's not 100% obvious to me)?

I think I'd vaguely prefer to add a blueprint for ember g modifier in ember-source, and have that check to ensure that ember-modifier is installed properly...

[chriskrycho]

@rwjblue

Hmm, this seems somewhat strange to me (and I missed the core team meeting referenced). Can you explain why? Is your expectation that every app will create at least one modifier of it's own (that's not 100% obvious to me)?

1. I think the answer is yes, most apps will want custom modifiers; but that’s an intuition rather than something I know for a fact, of course.
2. How is this different from (a) adding a @glimmer/modifier which is in the default blueprint or (b) things like having @ember/component/helper in the default?

I think I'd vaguely prefer to add a blueprint for ember g modifier in ember-source, and have that check to ensure that ember-modifier is installed properly...

Without (necessarily) implying argument, how is that an improvement over just shipping the capability out of the box?

---

Is the concern about adding bloat to the generated app build if it's included by default, or something else? Assuming we ship it as a v2 addon (pulling in the upcoming v4 version of the release, likely this week), which will in turn get resolved by way of ember-auto-import (if I recall correctly), doesn't that resolve that concern?

[chriskrycho]

Oh: the reason for adding ember-modifier rather than @glimmer/modifier was twofold:

1. @wycats noted that the work he has been doing with Starbeam indicates we may want a somewhat further-refined version of the modifier API in the future, and that he would prefer to avoid having to deal with a migration story of different versions of @glimmer/modifier vs. being able to tell people to migrate from ember-modifier to @glimmer/modifier piecemeal over time.

2. @kategengler noted that, if we want to avoid that churn, and we also don’t want to put it in @ember/modifier (which no one really wanted), then the actual minimal amount of churn here is simply to add ember-modifier as is.

[chriskrycho]

A data point on the expected usage level: ember-modifier has a similar number of downloads/month as @glimmer/tracking.

ember-modifier	@glimmer/tracking
554,711	559,886

This would not by itself be a signal to make it official, but it is a good data point for whether "most" apps will use it.

[rwjblue]

How is this different from (a) adding a @glimmer/modifier which is in the default blueprint

I agree, I don't think it is fundamentally different (other than the current design doesn't really support Glimmer.js or GlimmerX users, for roughly no good reason).

How is this different from (b) things like having @ember/component/helper in the default?

We don't have these in the default blueprint though (because they are not real packages).

---

After thinking about this a bit more, I think my main issue is that we aren't being prescriptive enough in this RFC. Some questions:

• Do we add this to the addon blueprint? If so, is it a devDep? Is it a prod dependency? Is it a devDep + peer dependency? Either way, the RFC needs to explicitly call out what we want. My personal preference would be to not add it to the addon blueprint (because I doubt "most addons will use modifiers"), but to ensure that ember g modifier in an addon works properly (where IMHO "properly" means devDep + peerDep).
• IMHO, if we are going to go the way this RFC is calling out (adding ember-modifier to the blueprint) then ember-modifier needs to be transitioned to a V2 addon before this RFC is implemented. If we don't, then we are making https://github.com/ember-cli/ember-cli/issues/9827 worse and knowingly adding it to the blueprint of all apps when we have to do a major "soon" is pretty bad...

[SergeAstapov]

@rwjblue for the first question, currently RFC says:

This RFC seeks to fill this gap in Ember.js' development mental model by providing ember-modifier in the blueprint. ember-modifier will be added to devDependencies, same as e.g. @glimmer/component.

we may edit it to make (of course if that path will be agreed upon):

This RFC seeks to fill this gap in Ember.js' development mental model by providing ember-modifier in the blueprint. ember-modifier will be added to devDependencies of both app and addon blueprints, same as e.g. @glimmer/component.

As for the second question, ember-modifier was already converted to v2 in https://github.com/ember-modifier/ember-modifier/pull/296, so the planned [email protected] @chriskrycho mentioned will be Embroider native.

[chriskrycho]

@rwjblue

My personal preference would be to not add it to the addon blueprint (because I doubt "most addons will use modifiers"), but to ensure that ember g modifier in an addon works properly (where IMHO "properly" means devDep + peerDep).

Wouldn’t it make more sense (both from an ease-of-implementation POV and an ease-of-figuring-it-out-to-maintain-it POV) to just leave it as a blueprint supplied by ember-modifier? What’s the situation where you would want ember g modifier to work in an addon which doesn’t have it as a dependency?

[rwjblue]

@SergeAstapov

This RFC seeks to fill this gap in Ember.js' development mental model by providing ember-modifier in the blueprint. ember-modifier will be added to devDependencies of both app and addon blueprints, same as e.g. @glimmer/component.

I do like your suggested tweak (quoted just below), but I think this only really works if we also ensure that ember-modifier's modifier blueprint also adds ember-modifier to peerDependencies if you generate a modifier in an addon (and possibly leveraging validate-peer-dependencies?). We really need to ensure that we properly encode the specific versions that your addon is reliant on (so that when we release a ember-modifier@5 folks will actually get feedback of incompatibilities).

[rwjblue]

@chriskrycho

Wouldn’t it make more sense (both from an ease-of-implementation POV and an ease-of-figuring-it-out-to-maintain-it POV) to just leave it as a blueprint supplied by ember-modifier?

I definitely agree that the simplest implementation is to "just" provide modifier blueprint in ember-modifier like you said, but it means that folks upgrading ember-source still don't necessarily get any help generating modifiers. Yes, I know this is obvious, but folks commonly update ember-source independently of other things; and if this RFC is merged we are saying that "the default stack includes support for ergonomically generating modifiers" and the fact of how we do that (adding ember-modifier to the app/addon blueprints) is just an implementation detail.

What’s the situation where you would want ember g modifier to work in an addon which doesn’t have it as a dependency?

I will provide an alternate question / example: ember g component works (and generates a @glimmer/component) even if @glimmer/component is not installed, what is wrong with that setup?

Also, to be clear, I think it would be 100% fine to actually do both:

• have ember-modifier actually own it's blueprint (assuming we codify what it should do RE: peerDependencies like I mentioned in prior comments)
• create an ember-source blueprint for modifier that ensures ember-modifer is installed and just run it's blueprint. This would still ensure the capability for generating modifiers "out of the box", and allow the ember-modifier package to own it's own implementation details. Seems win/win to me...

[rwjblue]

Oh, also, FWIW I think what I'm saying in https://github.com/emberjs/rfcs/pull/811#issuecomment-1124007886 really ought to apply to @glimmer/component as well. Without some mechanism for knowing which version of ember-modifier (or @glimmer/component) a given dependency actually relies on, our community just can never absorb a major release of those packages (because it would be roughly impossible to even know what dependencies need to be updated/bumped/etc and which are already compatible).

[chriskrycho]

@rwjblue I think you have an intuition/some experience/a lot of knowledge here that isn't obvious to me reading the conversation! Why could we as a community not absorb changes there? I'm asking because, assuming Embroider v2 addon format versions of these packages, it seems to me that the only reason the complication would exist is if we don't just add them to the package and make blueprints do special things (otherwise we would just cut a major and people could upgrade piecemeal: it is only Classic builds' Highlander Rules which would break that?)… but I assume that's because I'm missing something that is obvious to you from having worked on this problem space. Can you expand on why this is true?

our community just can never absorb a major release of those packages (because it would be roughly impossible to even know what dependencies need to be updated/bumped/etc and which are already compatible)

[chriskrycho]

I have put this on the agenda for tomorrow's Framework team meeting!

[rwjblue]

@chriskrycho

Why could we as a community not absorb changes there? I'm asking because, assuming Embroider v2 addon format versions of these packages, it seems to me that the only reason the complication would exist is if we don't just add them to the package and make blueprints do special things (otherwise we would just cut a major and people could upgrade piecemeal: it is only Classic builds' Highlander Rules which would break that?)… but I assume that's because I'm missing something that is obvious to you from having worked on this problem space. Can you expand on why this is true?

The current RFC prose states that ember-modifier will be in devDependencies, if we go with that (and don't do any of the changes I mentioned in https://github.com/emberjs/rfcs/pull/811#issuecomment-1124013993) addons would start having addon/modifiers/** and there would be no dependency relationship (dependencies/peerDependencies) that the package manager could leverage. We have exactly the same issue (as I mentioned in https://github.com/emberjs/rfcs/pull/811#issuecomment-1124016278) with @glimmer/component and @glimmer/tracking. Addons sometimes list those as dependencies, some times as only a devDep, and other times as both devDep + peerDep. Leaving this decision to individual addon authors to figure out, is leaving a massive (loaded!) footgun in place.

We must address this with official guidance, and this RFC is the right place to start. The two simple options are: make it a dependency, make it a devDep/peerDep. Both of those have trade offs, I personally think peer dep is the right relationship here, but at the very least this RFC must say something about this.

[jelhan]

We must address this with official guidance, and this RFC is the right place to start. The two simple options are: make it a dependency, make it a devDep/peerDep. Both of those have trade offs, I personally think peer dep is the right relationship here, but at the very least this RFC must say something about this.

I agree that this must be addressed. But I'm not sure if this RFC is the right place to do so. It seems to not only affect newly added ember-modifier package. As you mentioned it also affects existing @glimmer/component and @glimmer/tracking packages. For me it seems to deserve its own RFC.

[chriskrycho]

@jelhan it may indeed need its own RFC, but we need to figure that out before introducing this unless we want to possibly cause a high degree of churn across the ecosystem. Note that it is specifically the act of introducing it to the default blueprint, with the need for guidance around what to do, which risks that churn: until we do that, it's in the same boat as any other addon in the ecosystem (and there are no planned API changes for when we bring it in, as it stands!).

[jelhan]

@jelhan it may indeed need its own RFC, but we need to figure that out before introducing this unless we want to possibly cause a high degree of churn across the ecosystem. Note that it is specifically the act of introducing it to the default blueprint, with the need for guidance around what to do, which risks that churn: until we do that, it's in the same boat as any other addon in the ecosystem (and there are no planned API changes for when we bring it in, as it stands!).

I fully agree.

Let me propose the following:

1. Create a RFC how dependencies like these (@glimmer/component, @glimmer/tracking, ember-modifier etc.) should be handled in addons in general.
2. Scope this RFC (and #812) to only add the dependencies to app blueprints for now to unblock it.
3. Have a follow-up RFC to add ember-modifier to addon blueprints until the general question is solved.

[rwjblue]

For me it seems to deserve its own RFC.

Totally agree!

Let me propose the following:

• Create a RFC how dependencies like these (@glimmer/component, @glimmer/tracking, ember-modifier etc.) should be handled in addons in general.
• Scope this RFC (and https://github.com/emberjs/rfcs/pull/812) to only add the dependencies to app blueprints for now to unblock it.
• Have a follow-up RFC to add ember-modifier to addon blueprints until the general question is solved.

This seems like a reasonable step forward to me, but it's important to me that we don't loose track of actually doing that RFC. Additionally, this RFC should be updated to explicitly state that the addon blueprint will not include ember-modifier (I know this is probably what you meant in the second bullet there, but I want to be very explicit there since the addon blueprint "extends" from the app blueprint).

[SergeAstapov]

This seems like a reasonable step forward to me, but it's important to me that we don't loose track of actually doing that RFC. Additionally, this RFC should be updated to explicitly state that the addon blueprint will not include ember-modifier (I know this is probably what you meant in the second bullet there, but I want to be very explicit there since the addon blueprint "extends" from the app blueprint).

That sounds reasonable! If that's would be the consensus, I think I can write up that RFC how dependencies like these (@glimmer/component, @glimmer/tracking, ember-modifier etc.) should be handled in addons in general and we could work on it separately.

[chriskrycho]

@SergeAstapov do you have time to update this RFC this week? I think @rwjblue's concern was the only blocking concern here so we should be able to nominate this for FCP again once that's resolved!

[wagenet]

Looks like this is waiting on @SergeAstapov, correct?

[chriskrycho]

That's correct!

[SergeAstapov]

@SergeAstapov do you have time to update this RFC this week? I think @rwjblue's concern was the only blocking concern here so we should be able to nominate this for FCP again once that's resolved!

sorry for the delay, added the requested tweak in commit https://github.com/emberjs/rfcs/pull/811/commits/808da756585338f7badceff969490982b621f51f

@chriskrycho @rwjblue please let me know if this does not match exactly what you were thinking/proposing.

[chriskrycho]

We discussed this at Framework today and decided to put it into Final Comment Period, with intent to merge it! Thanks, @SergeAstapov!

[jelhan]

We discussed this at Framework today and decided to put it into Final Comment Period, with intent to merge it! Thanks, @SergeAstapov!

What is the status on this? Label for final comment period has not been added. And it has not been merged.

[chriskrycho]

Whoops! We've been talking about a bunch of things and just forgot to circle back to this. I'll do an async check on Monday and it should get merged then!

[chriskrycho]

Folks have been busy, but I have merging this at the top of the agenda for this week's Framework team meeting; it should get merged then!

[Panman82]

~~Could RFC #652 impact this RFC's API design?~~ Sorry for the noise, I don't think it is super impactful. Any API additions from #652 would have to be implemented in several areas anyway. 🤐

[wagenet]

When we merge, we'll have to manually trigger the advancement PR since this one affects two RFC files.