Element Modifiers #811

SergeAstapov · 2022-03-29T20:17:58Z

This RFC supersedes the original RFC #353 "Modifiers"

NullVoxPopuli · 2022-03-29T20:29:41Z

text/0811-element-modifiers.md

+- If we merge [RFC #757 "Default Modifier Manager][rfc-0757],
+  it may seem redundant with this RFC.
+- There is the potential for confusion between the `ember-modifier` package and
+  the `@ember/modifier` package which ships as part of `ember-source`.


what if ember-modifier is re-exported from @ember/modifier? adding Modifier and modifier to the exports from @ember/modifier?

@NullVoxPopuli this is meant by "Integrate ember-modifier into the @ember/modifier package directly." in "Alternatives".
I can expand that to be more specific

yeah, like, I think there are some pretty good ergonomics to that alternative, and I'm curious what others think.

Calling that out is good, I totally forgot about the similar import paths 😅

(good RFC, @SergeAstapov !!! <3)

@chriskrycho may know more about the potential downsides of this approach.

From my perspective, this would be logical to have imports like

import Modifier, { modifier } from '@ember/modifier';

as it would follow what we do with helpers, where we have imports like

import Helper, { helper } from '@ember/component/helper';

tangent, but I think the /component/ part of the helper imports is weird 😅

This is one of the questions I intend to talk through at the Framework core team meeting on Friday! One of the reasons is that there is open design question going forward: should this kind of thing be in @ember/* or @glimmer/*?

oo, good point. probably makes more sense for @glimmer/*, tbh -- even though ember feels like the place to be only because it feels more maintained. 🙈

text/0811-element-modifiers.md

chriskrycho · 2022-04-01T19:45:19Z

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.

rwjblue

I personally would love to see this come from @glimmer/modifier, what do folks think?

Windvis · 2022-04-13T16:09:34Z

text/0811-element-modifiers.md

+
+## Alternatives
+
+- Introduce [RFC #757 "Default Modifier Manager][rfc-0757]


I think this is the direction we should go in. ember-modifier's function-based modifiers feel like a stop-gap solution for the default modifier manager and it would be better to simply focus on that RFC instead. It would also be consistent with the already merged "Default helper manager" RFC and is one less thing we would have to import.

I realize adding the modifier function would be the modifier equivalent of the already existing helper function, but I think that should be deprecated once the default helper manager is shipped 😄.

The scope of this RFC could then be reduced to only the class based modifier API of ember-modifier which ideally, as mentioned in the RFC and by others, would be exported from @glimmer/modifier or @ember/modifier.

Thanks for writing this RFC ❤️!

chriskrycho · 2022-04-13T16:58:41Z

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

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.
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 Default Modifier Manager #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!

chriskrycho

One comment about the suggestions I'm making here:

This is explicitly not setting a precedent for adding other Ember ecosystem addons to the default blueprint, and should not be taken to invite RFCs to that end. The Framework Core team discussed the need for this primitive and the best path forward and then asked @SergeAstapov to write this RFC.

text/0811-element-modifiers.md

rwjblue · 2022-04-29T18:46:24Z

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 · 2022-05-10T13:45:03Z

@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)?

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.
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 · 2022-05-10T13:49:06Z

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

@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.
@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

Thanks for making these changes! Looks good to me. I have it on the agenda again for Friday's core team meeting!

chriskrycho · 2022-05-10T14:15:11Z

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 · 2022-05-10T15:58:12Z

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 Convert to v2 format addons in app blueprint ember-cli/ember-cli#9827 worse and knowingly adding it to the blueprint of all apps when we have to do a major "soon" is pretty bad...

SergeAstapov · 2022-05-10T16:08:05Z

@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 ember-modifier/ember-modifier#296, so the planned ember-modifier@4.0.0 @chriskrycho mentioned will be Embroider native.

chriskrycho · 2022-05-10T16:24:15Z

@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 · 2022-05-11T16:45:22Z

@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 · 2022-05-11T16:52:18Z

@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 · 2022-05-11T16:54:40Z

Oh, also, FWIW I think what I'm saying in #811 (comment) 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 · 2022-05-12T01:31:29Z

@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 · 2022-05-12T19:32:29Z

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

rwjblue · 2022-05-13T19:12:11Z

@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 #811 (comment)) 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 #811 (comment)) 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 · 2022-05-14T18:00:59Z

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 · 2022-05-17T18:06:10Z

@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!).

chriskrycho · 2022-10-07T18:37:37Z

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

jelhan · 2022-11-20T00:19:56Z

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 · 2022-11-20T00:46:51Z

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 · 2022-12-01T14:51:04Z

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 · 2022-12-02T20:28:21Z

~~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 · 2022-12-02T20:49:38Z

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

jelhan · 2022-12-02T21:05:08Z

~~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. zipper_mouth_face

We expect that we will add additional timing capabilities as part of addressing #652. I hope publishing such a RFC in the next week.

The new low-level primitives providing more granular control of modifier timing would need to be exposed in high-level APIs provided by ember-modifier somehow. But how such a high-level API could looks like, could and should be explored in follow-up PRs. I don't see it as a blocker for this one. Especially as ember-modifier package in blueprints is only an interim step towards a @glimmer/modifier package.

Advance RFC #811 "Element Modifiers" to Stage Ready for Release

Advance RFC #811 `"Element Modifiers"` to Stage Released

At emberjs/rfcs#934 we're tracking an effort to implement the RFC emberjs/rfcs#811. In that RFC (at https://github.com/emberjs/rfcs/blob/master/text/0811-element-modifiers.md#how-we-teach-this) there are two things which are suggested for documentation in the guides: * Update references that mention `ember-modifier` must be installed, since it ships in the default blueprint (confirmed at https://github.com/ember-cli/ember-new-output/blob/master/package.json#L50) * Introduce the class-based modifier API. In this PR I've linked to the upstream documentation at https://github.com/ember-modifier/ember-modifier#usage to execute on that suggestion. I don't believe we need to laborously explain the class-based APIs to guides readers, it just distracts from some otherwise straight-forward use cases and examples.

Advance RFC #811 `"Element Modifiers"` to Stage Recommended

SergeAstapov mentioned this pull request Mar 29, 2022

Add ember-modifier dependency SergeAstapov/rfcs#1

Closed

NullVoxPopuli reviewed Mar 29, 2022

View reviewed changes

chriskrycho self-assigned this Mar 29, 2022

bertdeblock reviewed Mar 30, 2022

View reviewed changes

text/0811-element-modifiers.md Outdated Show resolved Hide resolved

text/0811-element-modifiers.md Outdated Show resolved Hide resolved

chriskrycho added the T-framework RFCs that impact the ember.js library label Mar 31, 2022

chriskrycho mentioned this pull request Apr 1, 2022

Local modifiers not working correctly ember-modifier/ember-modifier#78

Closed

rwjblue reviewed Apr 6, 2022

View reviewed changes

MelSumner requested review from rwjblue and chriskrycho April 8, 2022 18:17

Windvis reviewed Apr 13, 2022

View reviewed changes

chriskrycho requested changes Apr 13, 2022

View reviewed changes

text/0811-element-modifiers.md Outdated Show resolved Hide resolved

text/0811-element-modifiers.md Show resolved Hide resolved

chriskrycho approved these changes May 10, 2022

View reviewed changes

chancancode mentioned this pull request May 13, 2022

Public router service #819

Open

SergeAstapov added 2 commits November 8, 2022 07:21

Merge branch 'emberjs:master' into ember-modifier

5491ac5

RFC 811: update links to Modifier RFC in Modifier Manager RFC

cd44292

wagenet added the S-Proposed In the Proposed Stage label Dec 2, 2022

chriskrycho added S-Exploring In the Exploring RFC Stage and removed S-Proposed In the Proposed Stage labels Dec 2, 2022

Merge branch 'master' into ember-modifier

a8254e7

wagenet added the Final Comment Period label Dec 2, 2022

wagenet added 2 commits December 12, 2022 16:35

Merge branch 'master' into ember-modifier

966f8f8

Merge branch 'master' into ember-modifier

678b523

wagenet merged commit 0c48183 into emberjs:master Dec 13, 2022

emberjs-rfcs-bot mentioned this pull request Dec 13, 2022

Advance RFC #0811 "Element Modifiers" to Stage Ready for Release #885

Merged

6 tasks

SergeAstapov deleted the ember-modifier branch December 13, 2022 01:14

SergeAstapov mentioned this pull request Dec 13, 2022

[RFC 811] Add ember-modifier dependency to app blueprint ember-cli/ember-cli#10109

Merged

wagenet added a commit that referenced this pull request Apr 7, 2023

Merge pull request #885 from emberjs/advance-rfc-0811

21e9de7

Advance RFC #811 "Element Modifiers" to Stage Ready for Release

emberjs-rfcs-bot mentioned this pull request Apr 7, 2023

Advance RFC #0811 "Element Modifiers" to Stage Released #928

Merged

3 tasks

gilest mentioned this pull request May 2, 2023

missing dependency breaks app after upgrading to v8.0.0 adopted-ember-addons/ember-file-upload#926

Closed

wagenet added a commit that referenced this pull request Jun 10, 2023

Merge pull request #928 from emberjs/advance-rfc-0811

8f7e082

Advance RFC #811 `"Element Modifiers"` to Stage Released

emberjs-rfcs-bot mentioned this pull request Jun 10, 2023

Advance RFC #0811 "Element Modifiers" to Stage Recommended #934

Merged

13 tasks

mixonic mentioned this pull request Jul 19, 2023

Update the guides for element modifiers ember-learn/guides-source#1946

Merged

ef4 added a commit that referenced this pull request Jan 19, 2024

Merge pull request #934 from emberjs/advance-rfc-0811

c8337e6

Advance RFC #811 `"Element Modifiers"` to Stage Recommended

kategengler mentioned this pull request Jan 22, 2024

Include modifiers by default emberjs/tracking-polaris#12

Closed


		## Alternatives

		- Introduce [RFC #757 "Default Modifier Manager][rfc-0757]

Element Modifiers #811

Element Modifiers #811

Conversation

SergeAstapov commented Mar 29, 2022 • edited Loading

NullVoxPopuli Mar 29, 2022

Choose a reason for hiding this comment

SergeAstapov Mar 29, 2022 • edited Loading

Choose a reason for hiding this comment

NullVoxPopuli Mar 29, 2022 • edited Loading

Choose a reason for hiding this comment

SergeAstapov Mar 29, 2022

Choose a reason for hiding this comment

NullVoxPopuli Mar 29, 2022

Choose a reason for hiding this comment

chriskrycho Mar 29, 2022

Choose a reason for hiding this comment

NullVoxPopuli Mar 29, 2022

Choose a reason for hiding this comment

chriskrycho commented Apr 1, 2022

rwjblue left a comment

Choose a reason for hiding this comment

Windvis Apr 13, 2022 • edited Loading

Choose a reason for hiding this comment

chriskrycho commented Apr 13, 2022

chriskrycho left a comment

Choose a reason for hiding this comment

rwjblue commented Apr 29, 2022

chriskrycho commented May 10, 2022

chriskrycho commented May 10, 2022

chriskrycho left a comment

Choose a reason for hiding this comment

chriskrycho commented May 10, 2022 • edited Loading

rwjblue commented May 10, 2022

SergeAstapov commented May 10, 2022 • edited Loading

chriskrycho commented May 10, 2022

rwjblue commented May 11, 2022

rwjblue commented May 11, 2022

rwjblue commented May 11, 2022

chriskrycho commented May 12, 2022

chriskrycho commented May 12, 2022

rwjblue commented May 13, 2022

jelhan commented May 14, 2022

chriskrycho commented May 17, 2022

chriskrycho commented Oct 7, 2022

jelhan commented Nov 20, 2022

chriskrycho commented Nov 20, 2022

chriskrycho commented Dec 1, 2022

Panman82 commented Dec 2, 2022 • edited Loading

wagenet commented Dec 2, 2022

jelhan commented Dec 2, 2022

SergeAstapov commented Mar 29, 2022 •

edited

Loading

SergeAstapov Mar 29, 2022 •

edited

Loading

NullVoxPopuli Mar 29, 2022 •

edited

Loading

Windvis Apr 13, 2022 •

edited

Loading

chriskrycho commented May 10, 2022 •

edited

Loading

SergeAstapov commented May 10, 2022 •

edited

Loading

Panman82 commented Dec 2, 2022 •

edited

Loading