-
-
Notifications
You must be signed in to change notification settings - Fork 406
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
Module Unification Packages (MU with Ember Addons) #367
Conversation
Since |
I think the problem with If we wanted to make it truly match import, it would need to change from: {{use thing from "somewhere"}} to {{use { thing } from "somewhere"}}i Because we're always accessing named exports. And the two levels of I guess this is worth calling out in the alternatives section. I can imagine just living with the gap between JS |
That makes sense to me. Also, if you used But i think the amount that is different is easily argued as a need for another word, with using a similar convention. |
So the |
@rmmmp I'm not sure how to read this statement:
When crossing any package boundary (via templates or JS) you must specify the package. For example I suggest an Ember Data API of the following: In an app, explicit package (in RFC: Explicit packages for service injections) // my-app/src/ui/components/foo-bar/component.js
import Component from '@ember/component';
import { inject } from '@ember/service';
export default Component.extend({
store: inject({ package: 'ember-data' })
}); In ember-data itself, implicit package (in RFC: Implicit packages for services) // ember-data/src/services/something.js
import Service, { inject } from '@ember/service';
export default Service.extend({
store: inject() // babel transform will add the `source` arg for you
}); Many people have commented that they rather like
Basically it would be non-standard and I don't suggest that there is a good solution here. |
Can I pass a string to the Is there a way to basically |
@mixonic got it. I think I was the one who misunderstood things. This paragraphs has a lot of fancy words in it that it confused me 😛
|
Correct, this is not suggested. The syntax section for I would like these import statements to be hints for improving Ember's template compilation and application packaging. If the source of the import is dynamic we lose lots of information.
Sure, you could explicitly import all the components from the package and the use the What is not provided are two things I believe you are implying. First, something like JavaScript's "import all named exports" feature: import * as name from "module-name"; This would then perhaps allow you to invoke a component with Sidestepping this and requiring statically named imports seems like a good idea for now. Maybe in the future Ember can decide build-time imports are reliable at runtime, but it just isn't true today. The second thing implied is that this renders the component {{use foo-bar from 'my-addon'}}
{{component computedPropertyWithStringOfFooBar}} However the component helper doesn't do this today. Strings passed to the component helper are resolved in the current package. A string passed to the There was discussion about if the component helper should be changed to support this. I decided against advocating for that change for a few reasons:
|
This is limiting (not in a good way). For local packages there’s a use for aliasing or renaming components. I think that having the Having the For clarification I don’t mean that all local component declaration should require a |
Overall very positive! 🥇 Two thoughts:
// I expected this:
{{use 'Select' from 'ember-power-select'}}
// instead of:
{{use Select from 'ember-power-select'}}
// To me, the latter is more similar to:
{{component variableThatResolvesToAstring}} Or have I completely misunderstood something? |
@mixonic thank you very much for clarification. {{component @name}} I totally agree that this works for now. Can't wait for MU now. |
@sandstrom I had that initial reaction too when watching mixonic's NYC meetup talk. I came around though for two reasons
anyway, just my two cents. Thanks for the work on this @mixonic 💯 |
@mixonic I'd love to see a migration path example that takes into account "shipping with sensible defaults" as I've described in great detail below |
It seems that as specified, {{use Select from 'ember-power-select'}}} Now suddenly the user has Perhaps it would be best to say that the only contents |
Great point @Kerrick! In addition to limiting |
if the prelude is limited to “use” import statements, does it being a special one off .hbs file cause more confusion then it being a config block or js file? Also, where does this file live? can it be scoped to a “level” i.e. the values in It’s mentioned that only one use can be done, and that “Once an addon has claimed a name in a prelude the app will not, in its own templates, be able to write {{use}} for that name without a re-assignment.” Shouldn’t the use from the individual template over write the prelude rather then the other way around? Isn’t that the point? Otherwise, if an addon developer adds a generic item to the prelude, we are kind of back in the same boat we currently are if we can’t override it on an individual basis. Or did i read that passage wrong? |
I think the prelude.hbs has a potential to cause confusion with application.hbs How should we describe things so this wouldn't confuse newcomers? |
This occurred to me over on the Modifier Managers RFC when I read a In the context of this RFC, how do these functions work when referencing a manager from an addon? Would it be something like |
I like to explicitly import the components and helpers I want to use in my templates, so I'm generally in favor or this. I even think that we could take this one step further and also require to explicitly import even components from your very app, but I think this is a good compromise for now. If next year we realize that explicitly importing components from your app can help with tree shaking it seems that it wouldn't be terribly hard to opt-in into a "strict mode" that enforces explicit imports. At the end in React apps imports are explicit and it's not a big deal. Also, this could open a door to lazy-load components when they are imported with a very similar syntax some day in the future. Also, regarding the prelude, it's supposed to be used for components or helpers that are used suuuuuper often, like the |
My main concern is that there is no way to enforce this, and if an addon developer can add all of their components to this file, which definitions take presedent over your own in your app, we are basically where we started. |
@webark Personally other than the helpers in ember-truth-helpers, I struggle to find any other example of something I use sooooo often that explicitly importing it would become annoying. Maybe But I can see people feeling otherwise. As long as addons cannot add things to your prelude implicitly (they obviously can if they use the |
Yeah, I'd argue that the behavior should be both discouraged and explicit; as long as it is both of those things, the user can trivially undo it if they so choose. |
As i read that, if an addon author defined a “not” helper, that would be something you could not redefine, and arevstick with there’s. And if two addon authors do the same, not sure what would happen. I get that this behavior should be discouraged, but without any enforcement, you lend yourself to the some of the same issues that drove the creation of this addon. I the pretending of use statements to some, or all template files, is something that if the community decides is a priority, a extra community addon could be created that would do just that, and people could opt in that way. I feel (personally) that the “prelude” feature shouldn’t be included as part of this effort. And if we hold to local helpers and components don’t need to be “use”d, then you could just create your own “t” helper that just inherits from i18n’s for example. |
Right, but the explicitness of the prelude is the other save: if you don't want something that an addon added to your prelude – since it's in the actual file |
hmm.. I did not read it as lines that where continually added to the consuming apps prelude. That makes more sense, and would be more manageable. Does this concatenation happen automatically? or would someone have to write there own custom post install hook in their addon to opt in. |
Yeah, re-reading I see where the ambiguity comes from there. It might be worth a small update to the text to indicate that an addon can only do that by running a post-install hook that updates the |
One of the main things that has been coming back up in the chat is loosing the ability to organize components in deeply nested trees. A few people have brought this up @Alonski, @lougreenwood, @NullVoxPopuli, sol and Robbo (both from Discord) and I think it's worth at least mentioning in this RFC. I do see the value in nesting purely from an organizational perspective, where things go into categories or areas and make it easier to reason about and navigate components. People that have mentioned this concern have many components, some 400+ I think. I encourage those people to give some concrete examples of where nesting has helped them, with specific names of nested components and how they are used (private or outside of parent components). Packages are a sort of solution, but they only support one level of grouping and require a few levels of folders to even get started, e.g. I'm for punting on nested groupings and revisiting in a followup RFC, especially since it was never advertised as a feature, but I do think it needs to be mentioned here if that's the way we are going to approach it. In that case, the classic mode should not be deprecated before there is a solution/consensus for nesting. |
From Discord in response to the discussion mentioned above:
|
We use nested components for namespacing. Most of these namespaces match either route groups for parts of our application or could be easily moved to in-app-addons from what I see. We'd probably migrate slowly for those as we migrate to angle brackets and MU folder structure. |
how can the addon then use this code from addon code? Is there a way to And the same for components? Is there any way to use a app component from addon code? @knownasilya isn't this already part of the original RFC?
Local relative lookup will work right? |
@knownasilya Thanks for speaking our concerns. |
it already does: https://gitlab.com/NullVoxPopuli/emberclear/tree/master/packages/frontend/src/ui/components/chat-entry :)
|
@luxferresum yes it is mentioned, but I think since packages is it's own RFC and it promotes local single level grouping, it should consider the nesting scenario explicitly. I don't think it's enough to refer back to the original RFC because the '/' was avoided in the actual component name, e.g. The original RFC threw out the idea of grouping all together, but this RFC supports one level of grouping. So far it seems like one level isn't enough for many people. |
i must have missed the nested part. Are you not able to use |
Is there a blocker to supporting a Here's the motivating example: we have a family of components which we'll be grouping together into a package and possibly extracting to a shared addon. In the old proposal, I'd have used name-spacing for that: If we had the ability to |
Can we support a short form of {{use "ember-bootstrap"}}
{{!-- we can use all the public package components with a short form use! --}}
{{bs-tooltip}} I think it also can be convenient in prelude. Playing around @chriskrycho's proposal above, I can think of something like this for namespacing: {{use 'olo-shared-components' as Olo}} |
I'm totally for the @chriskrycho proposal, but only with a name. An import all from addon, like |
@chriskrycho I think the fact that ECMA modules already support |
I just want to echo @chriskrycho's suggestion to allow I think its also worth noting that this goes some way to address your concern @mixonic
When you can export groups of components with an alias you dramatically reduce the length and number of import statements, while simultaneously adding a lot of context as to which components are grouped and indicate that they work together, or have the same source (I've noticed this pattern becoming more common in big react project component imports). Allowing |
<Alias.Form>
<Alias.Input />
</Alias.Form> which I am a big fan of. :) |
Maybe I'm mis-understanding, but would |
No, the rules around nesting would be unchanged; the only thing I'm suggesting with the addition of |
Ok, in that case, +1 me in :) - this could be very powerful for allowing nice semantic names for components in templates. I can see this being very useful for the general Ember teaching story & guides too. Also, if we begin to see common alias names emerge for component aliases in the community, it's another point towards the "It's easy to onboard onto an Ember project" story. |
@chriskrycho would this work?
|
I think a follow-on RFC would be a good place to add that feature. One downside of this feature is that since I'm not convinced there is a case where Basically we're considering a {{use * as FormHelpers from '@org/form-helpers'}}
<FormHelpers.Form>
<FormHelpers.Input />
</FormHelpers.Form> vs. (and there are a few ways this could be done in practice) {{use form-helpers from '@org/form-helpers'}}
{{form-helpers as |f|}}
<f.Form>
<f.Input />
</f.Form>
{{/form-helpers}} @samselikoff said:
I believe @wycats and @dgeb are exploring a design on that front. I think there are notable challenges but it is plausible. For example we need a system for services (and arbitrary lookups) to come from addon's as well, so perhaps services become class-based injections instead of name-based? That removes flexibility from the DI/container system. And IMO front-matter or some other syntax-switching would look weird once you try to build single-file components. The Vue system still means swapping to a class-based system, and implies the need for a JS file just to reference components from another package: I think that is a bit clumsy. If there is some uncertainty about using I've been on leave for a few weeks and missed an informal conversation at the most recent F2F that seems to have made an impact on some core peeps. I'm pretty happy with this RFC (I think clarifications to the Ember Data migration section would be nice but the conversations here have covered most of that). It would probably be helpful if some framework core people engaged here. I'll raise it at the next standup I attend regardless. |
Just wanted to follow up on what @mixonic said ASAP. It's true that @wycats and I and several others on the core team discussed the implications of using ES imports for templates at the last F2F in Chicago. The more we discussed it, it seemed obvious to us that we won't have legitimately done our due diligence on this design without fully considering this potential solution. At this point we're doing some exploratory work right now, which involves a preliminary design and pulling on threads in the MU design to understand its full implications. We will surface this work as soon as possible if it seems to us like it's got a chance of working well. Either way, we'll report back here so that we can engage with the community, make a final decision, and get some traction on the implementation ASAP. Thanks to everyone for your patience in what has turned into a very long process. |
I'm leaving this thought here instead of discord for posterity... One of my concerns around MU/Packages is that it seems to remove the ability to use directories for organising components. Personally, I like to group many components which are used in a similar context together, for example, I currently have a
When I heard about MU I felt quite upset that this pattern is seemingly disallowed because of the no nested components and the switch to local/global components. But since then I've started on a new project and I've been exposed a lot to contextual components through some work I did on My idea is to make use of the factory pattern for components. So, in my form example above - currently I invoke a form by doing However, with the factory pattern, I would create some new
// components/form-factory.js
export default class FormFactoryComponent extends Component {
someFancyForm="components/forms/some-fancy-form";
someOtherJuicyForm="components/forms/some-other-juicy-form";
@argument
@type("string")
form = null;
@argument
@required
model = null;
@computed("form")
get formComponent() {
// TODO: validation
return this[this.form];
}
}
Developing this idea further, these simple Factory-like components could use some type of custom component using RFC 213 to minimise performance impact - I expect in my case I'll make use of a fair number of these component factories across my application to allow me to keep my semantic component organisation - knowing that these are minimal components likely with no runtime hooks or bindings to add weight could be a bonus. I'm going to test out this idea over the coming days, but I just wanted to share my idea and get any feedback - maybe I missed something obvious, or maybe people are already doing something similar? |
The idea of the |
I'm not sure that's the current intention, from all of the discussions I've seen here and in Discord If I'm wrong, that's even better 🎉 |
Bringing some discussion from Discord and fresh 👀 to this RFC. I think a lot of the discussion here has been about the particulars of the I think since the syntax of While the rest of this package RFC requires an import API for addon/package component resolution, IMO it makes more sense to split those out (and maybe indicate any details/changes needed here) |
// inject src/services/geo.js | ||
geo: inject(), | ||
|
||
// inject node_modules/ember-stripe-service/src/services/store.js |
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.
// inject node_modules/ember-stripe-service/src/services/store.js | |
// inject node_modules/ember-stripe-service/src/services/stripe.js |
Good morning! I'm closing this RFC today in favor of SFC & Template Import Primitives and other possibly-to-be-written RFCs focused on a new filesystem layout for Ember. The Putting aside the exact design addressing imports: There was some suggestion that this RFC could simply strip out the An "explicit import" system means only explicit imports. In the same way this this RFC suggested removing the implicit resolution of an MU package (an addon's) "main", explicit imports would replace the need for local lookup and even top-level component lookup in a package. You import any component you need, all the time, and always based on the path on disk. That design obliterates most of the clever bits in the original MU RFC:
Furthermore as far as creating a migration path for Ember app code: Templates with no implicit scope don't care about resolution rules, and so don't intersect with a new filesystem layout. Their design doesn't require we put components in That leaves the migration path for Ember applications to a new filesystem layout wide open. We need to figure out a design for templates that considers layout on disk, and might use it as a carrot, but it decouples templates and component entirely from a resolvable system. So what is left for MU, or any resolving system, to tackle? A major fault in Ember's addon story is the merged Addons in Ember often provide only two kinds of resolved modules: Services and components. I've seen models provided as well. I'm unsure about how public the API is, but Ember developers can already inject a service from an addon's export default EmberComponent.extend({
session: inject('my-addon@session')
}); We could RFC the export default EmberComponent.extend({
session: inject({ package: 'my-addon' })
}); ...but if this is the only impact of that change I doubt it is worth adding In general Thank you to the community members and contributors who helped hash out the nitty-gritty of this design! By fleshing out these ideas so concretely we demonstrated the best of the RFC process: We rejected the design before it caused churn for our apps. |
An iteration on and replacement of #309
Rendered