-
-
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
Module Normalization #124
Module Normalization #124
Conversation
Looks very good in the general, just a small thing: How would routable components look with the proposed structure? |
@martndemus - The |
Thanks @martndemus
To expand upon @rwjblue's comment: Also, a new directory would be required for a transition strategy (i.e. to allow Ember CLI to understand both the current and new module structures simultaneously).
They would co-exist alongside routes in a nested namespace. It's TBD whether they would use the |
I'll reread this again but WRT addons mixing into the consuming app namespace: Today a consuming app can override consumed addon the addon is responsible for displaying said template. How does that work in this model? We rely heavily on this behavior to extend addons (which we use heavily as a pre-engine way of breaking up our large app) |
I really, really like the dot naming convention with namespace, name, and type. It's the first naming/module convention that has felt spot on. The bucket idea where some folders contribute to the lookup path and others (prefixed with |
module lookups by the resolver. | ||
|
||
Construction of this map will allow for detection of any module name conflicts. | ||
In the case of any conflicts, Ember CLI should throw an exception. |
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.
Is this map going to be created at build time? I'm pretty convinced that none of this string parsing should happen at runtime. There is no reason you can't build the map at build time and hydrate the resolver with the precomputed map.
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.
Yep, we absolutely plan to do this at build time so the runtime resolver has much much less work to do.
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.
👍
We've locally implemented almost this exact pattern for the last two years and have really enjoyed using it. We didn't have the name.type pattern implemented in our custom resolver, but we also haven't felt a strong need for it as auth.js was enough of an identifier. It feels like the name.type pattern could help clarify some files purpose, but there are still components, such as new-post, that have files with type only names (component.js and template.hbs). I would rather have the components nested in a folder than flattened out to include the name and type in it, but it does feel there is a discrepancy for files in some buckets verses others. |
@scottmessinger I've probably done a poor job explaining the concept of "normalized" modules because I actually agree that this is important. ES modules will still mirror file locations on disk, and can be imported into other modules accordingly. Therefore, your imports would still include any bucket segments in module paths. However, the resolver will be looking up modules based upon the normalized form of module paths. The resolver will reference a mapping maintained between normalized modules and file-based modules. This mapping will be created at build time to minimize work at run time. |
Overall this looks amazing and addresses all of the issues that I had mentioned in #44. |
|
||
## Co-existence with current module and file structures | ||
|
||
It will be necessary to allow a project to migrate its files from the current |
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.
Is there plans to EOL existing project layouts in 3.0.0 or is the plan to support 3 project layouts?
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.
EOL hasn't been specifically discussed, although I'm sure that existing project layouts will be supported for quite a while. It seems critical to be conservative here, especially regarding addons which may not be under the control of a project's owner.
How do normalized and file based paths affect buckets? The explanation above needs an example I think. |
👏 👏 There is a lot of great stuff in here and I think it strikes a good balance between a lot of tension points. Huge 👍 to the "buckets" idea. We've avoided the current pod structure for two reasons, both of them feel solved by this RFC.
One question I do have is regarding styles (CSS, SASS, etc). What do you see is the impact for style based addons (eg |
I like the bucket idea.. but for routes that have child routes.. should those child routes not go within a new I have a feeling that if the fractal pattern is solid and consistent throughought, then refactoring and moving files around would be much simpler? |
templates. | ||
|
||
If an addon namespaced as `magic-ui` has a root-level `select/component` | ||
module, then that component can be rendered as `{{magic-ui::select}}`. |
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.
How does this work with nested component modules? If the magic-ui
addon has a component corresponding to the module my/awesome/select/component
, what does the invocation look like in hbs?
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.
@csantero One of the design constraints seems to be a blocker:
No slashes in component names. The existing system allows this, but we don't want to support invocation of nested components in Glimmer.
I wasn't present when this constraint was formulated, so I don't know the full rationale. However, it seems technically possible to support nested namespaces à la {{magic-ui::my:awesome::select}}
.
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 biggest blocker on this syntax for me. Across all projects I've been involved with this is an unmanageable breaking change with no path forward without support for better component namespacing.
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.
Agreed. Didn't realize that this was one of the limitations.
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.
@runspired - This is absolutely not a breaking change. All existing structures will continue to work.
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.
@rwjblue I left a longer note below. This may not be a breaking change for curlies, but as things stand curlies would be relying on (assumed to be deprecated) lookup patterns, and there is no path forward from curlies to angle bracket components that use this pattern.
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.
To my knowledge there are no plans to deprecate curly components. Curly components will have capabilities that are unavailable in angle bracket components (i.e. tagless/fragment)...
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.
while I get there are a few behaviors we will need to leave curly components behind for, seems odd to box some forms of components into curlies just to make them more composable..
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.
My general experience sides with @dfreeman, the vast majority of my current nested components would be solved very nicely by the current proposal.
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.
Our company has built up a large inventory of utility/infrastructure components that live in a couple monolithic shared addons used by our apps. There are so many of these components that we feel that nesting them is a big win for usability. If nested namespace angle-bracket components aren't possible, we will have to either suffer the headaches of having dozens of components at the top level, or just forgo the potential benefits of angle brackets.
@grapho as much time as I've spent on this, keeping all the details in one's head is quite tricky. I confirmed my understanding with @dgeb and let me see if I can answer your query:
From the RFC:
Emphasis mine. Buckets are an ergonomic addition, not a semantic one. At the core of this proposal, they have no meaning. For example take this route:
Note that the word
But this works with or without
The above would describe a route And note that the Later in the Buckets section @dgeb says:
So in Ember-CLI, we would lint/assert that you only put modules of the type We do not need to endlessly nest |
Overall, I like this approach. I love the name I share @scottmessinger's reaction about the buckets and the I love the idea of being to have As for routable components, I don't think any particular distinction in folder structure is necessary. I've found myself preferring the react terminology of "container" components and think that would work well with the component names, i.e.,: This is my first RFC comment. I hope it's helpful! |
Overall I think this is a step forward over other "pods" RFCs, but I think this is incomplete in a critical area because of the desire to "clean up" the template invocation of angle-bracket components. Here is an example of a common and useful structure that does not seem possible to convert nicely into this new structure.
This component use pattern doesn't seem possible in the proposed namespace pattern without sub-namespacing, the implementation of which according to @dgeb's comment above isn't guaranteed. e.g.
These components are problematic because they are all part of doesn't work
does work
This gets us right back to the same problem we had that led us to dream up pods: giant unmanageable component directories, albeit (slightly) improved by the top level namespace breakout and some components being able to be local. Interop with engines and the ability to break out portions of an app would be a lot nicer if namespaces carried down the route hierarchy (e.g. were nestable) instead, but for deeply nested pods I get that that's a performance cost we want to avoid. Although verbose (and more verbose maybe than needed), this syntax below is something I really liked about the current pods structure, because it makes the component's location explicit. You don't need to reason about whether a component is in a local, namespace, or global directory, and it let's components
I would like to decrease the verbose nature of the above, ideally it be nice to be able to do Two colons?I'm not sure why two colons were preferred to one in the syntax, but at least at the moment Angle Bracket ComponentsHow does this work for angle bracket components? Is it identical?
It seems to me that if the above is the syntax for angle brackets, then there isn't much reason why e.g.
|
Another thought, I would prefer that all of a type go into a bucket. example 1
not example 2
By enforcing buckets I think we not only make our lives easier programmatically, but we increase the developer ergonomics a great deal. In example-2, I don't know that |
Wouldn't it be weird having two different naming conventions, Doesn't the |
@runspired That could work if there is a 1:1 mapping between buckets and types allowed in each bucket. This option was explored in an alternative proposal that was dropped. This form could technically coexist with the normalized form if we instead dropped support for multi-type buckets like |
@mattmarcum this seems like an entirely valid concern, and is the reason I added this unresolved question to the RFC: "Should we tighten the flexibility of the alternative patterns? For example, we could require top-level buckets or emit warnings when certain naming patterns are used with certain types."
It's a more concise format that avoids repetition. And the "titlebar problem" isn't present in all editors, so it won't be a major issue for some. With all that said, we may come down on the side of choosing a single convention rather than allowing all the flexibility presented in this RFC. Thanks for sharing your perspective. |
I don't think we should be designing a folder structure around an issue with file editors. Those issues can be and should be addressed on the editor side. This is possible now, and I know Atom has some plugins that address this. |
@dgeb I prefer multi-type buckets too. I think I worded what I meant poorly. Basically, what I'm suggesting is that only routes can folders within another route, all other "types" must be in a bucket of some form. I think This would make your organization follow your ui, and help keep the ui nesting / organization clear at the directory level. |
@mattmcmanus I don't particularly like the bucket prefix either, but we do need some rule to prevent conflicts with namespaces. I'm open to alternatives. One alternative I've considered is to allow buckets only at the top-level, with no prefix, so the the entire top-level set of directories "drops out" of the normalized form. This of course would also require usage of those top-level buckets, since every directory at that level would need to be considered a bucket, not a namespace.
Definitely - thanks for chiming in! |
Just to ask, if
Overall I love this RFC. |
I don't have much to add that hasn't already been said, but I love this RFC. Great solution, and it gives us a path forward with backward compatibility until Ember 3.0. (and likely after with an addon). |
@rwjblue @dgeb @stefanpenner In general I'm 👍 with what this RFC lays out. I'm wondering if something more can be done with the mapping of normalized module name to path on disk. One of the challenging things about the Linker/Packager stuff is there is no "main entry" of ember apps to start dependency resolution from. If we generated a main file that imported an app/engine's files like the following we could solve this problem and also achieve the mapping.
To prevent evaluating all of the objects up front we could create lazy accessors to the modules like @stefanpenner experimented in emberjs/ember.js#11576. Or we bring back the minimatch The only other tricky thing is allowing addons to diverge their package name from their namespace. While it's still doable the Packager/Linker stuff is going to need to do a song and dance to figure out the mapping of |
i think we should disallow this entirely
aren't we able to just treat the whole engine/app as a retained? Or am i mis-understanding. |
@stefanpenner We can connect all files in an app/engine as root nodes in the graph. The advantage of having the This is getting orthogonal but with a |
don't we already need to treat everything in an engine as root nodes? Most files aren't connected via module imports, as they are loaded by DI. |
Yes you have to root all of the files. I'm just saying you can cleanup the walk if you can start from a real main instead of a "virtual main" that requires a filter and connect pass. Ideally this is the simplest code to build the graphs.
When you go find reachable/unreachable/intersection of the graph you have to run something like postorder on each node that is part of the root. This means you must individually track the root nodes. With a |
For addon namespaces, is it possible to use the same namespace for multiple (logically related) addons? Alternatively, could multiple "main" components be exported from an addon? We have an addon that acts as a core component library and a number of extension addons that can be optionally included and I'd prefer that all of these either follow the |
some thoughts:
|
Thanks to all for your feedback. At the last face-to-face meeting, the core team nixed the concept of "buckets" as defined in this RFC. It was decided that the sleight of hand required to allow buckets to be used for organization only, and not for resolution, could create confusion. Modules could conflict across buckets, because they I am working on a new proposal that builds on many of the concepts in this RFC, but replaces buckets with a new "collection" concept. Collections provide the same organization benefits as buckets but do not drop out of normalized namespaces or the resolution process. Instead, rules for collections are baked into the logic of the resolver. Everything will still be declarative, statically analyzable, and extensible. I plan to post this proposal as a fresh RFC in the very near future. I'll try to address many of the points raised here in the new RFC. But if I miss any, and they still apply, please re-raise them in the new PR. |
@dgeb thanks for stewarding this process. |
Closing in favor of #143 |
Rendered