Skip to content
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

Component Templates Co-location #481

Open
wants to merge 8 commits into
base: master
from

Conversation

@chancancode
Copy link
Member

commented Apr 23, 2019

Rendered

legal values for this option are `@glimmer/component` (aliased as `-gc`),
`@ember/component` (aliased as `-ec`), `@ember/component/template-only`
(aliased as `-tc`) or an empty string (aliased as `--no-component-class` and
`-nc`).

This comment has been minimized.

Copy link
@GavinJoyce

GavinJoyce Apr 23, 2019

Member

Will we be referring to @ember/components as "classic components" once Octane lands? If so, perhaps we should use -cc instead of -ec for @ember/component? This would also mirror the -cs classic structure below, "classic" would then consistently mean the old way of doing things.

@GavinJoyce

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

I'm very excited to see this RFC, thanks for writing it.

A constant pain point for new people joining our company and working on our app, and with Ember for this first time, has been the relative difficulty and mental load of navigating between .hbs and .js files. This RFC will significantly improve that pain point. It will also make teaching Ember easier as the file layout rules will be much simpler and will no longer require a detailed explanation.

This also seems like a nice incremental step towards module unification.

@SolPier

This comment has been minimized.

Copy link

commented Apr 23, 2019

Its not really this RFCs subject, but could it open a way for a controller/template collocation as well ?

As @GavinJoyce stated, these files distance in the project tree is a relative painpoint of learning and practicing ember. As the coupling of controller/template is conceptually the same as components js/hbs, and as the technical solution may be similar (from afar), they may be an opportunity to hit two birds in one shot ? I mean, benefiting of the simplification of the two template+js couplings.

(Edit: Now that I think about it, this coupling is between routes/components/templates... not that easy actually)

@lifeart

This comment has been minimized.

Copy link

commented Apr 23, 2019

Hm, we use ember g component component-name --pod and get same structure (without podModulePrefix)

  • it's working out of the box

image

@ggayowsky

This comment has been minimized.

Copy link

commented Apr 23, 2019

Will this RFC affect applications using pod structured components?

It was unclear to me, but reading the RFC I got the feeling that an application using the pod structure:

app
|-- components
|    |-- component-1
|    |    |-- component.js
|    |    |-- template.hbs
|    |-- component-2
|    |    |-- component.js
|    |    |-- template.hbs
...

Would need to rename everything to use index.js and index.hbs i.e.

app
|-- components
|    |-- component-1
|    |    |-- index.js
|    |    |-- index.hbs
|    |-- component-2
|    |    |-- index.js
|    |    |-- index.hbs
...

@amyrlam amyrlam referenced this pull request Apr 23, 2019

Merged

The Ember Times No. 95 - April 26th 2019 #89

6 of 18 tasks complete
@SolPier

This comment has been minimized.

Copy link

commented Apr 23, 2019

Here the js+hbs aren't nested in a directory int the PR.

pods:

app/
|-- components/
|    |-- component-1/
|    |    |-- component.js
|    |    |-- template.hbs

this PR:

app/
|-- components/
|    |-- component-1.js
|    |-- component-1.hbs

This is less of a change from the current structure (the component.js doesn't changes at all by example, nor its name nor its location).
Only the template location changes.

@kovalchik
Copy link

left a comment

Really excited to see this aspect of the file structure moving forward, even if it's not the full module unification implementation! I have a few minor questions, but overall I really like that we're incrementally moving towards a more coherent and flexible file structure. Thanks for taking the time to put this together.

files inside a directory to have the equivalent semantics. In the example
above, it could also be structured as `app/components/foo-bar/index.js` and
`app/components/foo-bar/index.hbs`. This allows additional files related to the
component (such as a `README.md` file) to be co-located on the filesystem.

This comment has been minimized.

Copy link
@kovalchik

kovalchik Apr 23, 2019

For the folder based layout, I understand why app/components/foo-bar/index.js represents the component’s class file; this generally makes sense from a module import perspective. I’m not sure I agree with app/components/foo-bar/index.hbs representing the template. This kind of makes semantic sense for template only components, but for non-template only components it creates two identically named files (with the exception of the extension) that both represent the component. This seems like a confusing convention to me, as JS modules typically have one entry point. Once tests are introduced to the folder structure and users update addons that provide functionality like CSS modules or Storybook tests, the index naming convention seems like it will make even less sense. If I were a new user, I'm not sure I would understand why there are two index files in this folder.

- foo-bar
  - index.js
  - index.hbs
  - styles.css  // index.css ??
  - tests.js
  - other.js

Many other component folder structures I’ve seen that utilize an index.js file, do so in the following way. Since Ember hooks up the component index for us, I don’t feel that we particularly need to adhere to an index naming convention, especially since it creates duplication with the template file name. That being said, I'm not totally opposed to the index.js/index.hbs proposal; I'm mostly nitpicking at this point. :)

- foo-bar
  - index.js  // re-exports component.js
  - component.js
  - template.hbs

This comment has been minimized.

Copy link
@Alonski

Alonski Apr 25, 2019

Member

For a project with 100 components. This means another 100 files on disk that take up space and need to be parsed during build time even if it is a one liner.
I actually like having the JS and HBS both be named index.ext
It shows the intent that they are used together. :)


We propose to allow placing a component's template adjacent to its JavaScript
file in `app/components`. For example, for a component named `foo-bar`, it will
be `app/components/foo-bar.js` and `app/components/foo-bar.hbs`.

This comment has been minimized.

Copy link
@kovalchik

kovalchik Apr 23, 2019

I have some concerns about supporting components in a folder (app/components/foo-bar/index.js) and also components at the top level of the components directory (app/components/foo-bar.js).

Which of these structures will the CLI default to? Will there be a config setting to enforce usage of one form over the other on a project-wide basis?

For my own projects, I would personally prefer the folder based approach, so I do appreciate the flexibility but it seems like having two file structure options reduces the consistency and could lead to developer confusion.

In particular, the non-folder based structure seems like it could quickly create a mess of the components directory, especially if other file types are introduced, via further folder structure refinements or addons. Although the following component files are located together, it's not particularly easy to parse what belongs to what.

- components
  - first-component.js
  - first-component.hbs
  - first-component.css
  - first-component-tests.js
  - first-component-other.js
  - second-component.js
  - second-component.hbs
  - second-component.css
  - second-component-tests.js
  - second-component-other.js

This comment has been minimized.

Copy link
@buschtoens

buschtoens Apr 24, 2019

I personally also much prefer the "pods" approach, exactly for the reason that it is easier to extend for, e.g. styles.css files or even further "private" files that are imported by the component.js.

@davewasmer

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

Thanks for this @chancancode! I really appreciated the historical context in this writeup.

I'm a big fan of co-location, but I'm concerned about this proposal. While I don't disagree with any of the design or implementation, I worry that the larger picture isn't addressed.

Understated cost of churn

The RFC readily admits this structure might change once again if / when some future form of Module Unification lands:

There is a small risk that we will subject the community to another migration when we finalize the replacement of Module Unification

I think this downside is understated here. I'm less worried about the time it takes to convert code between formats, and more worried about the mental churn. Ember already has two (ish) supported filesystem layouts - this RFC adds a third (but for components only!) which we are acknowledging will likely be obsoleted.

If / when Module Unification lands, it seems like a painful story to explain to a new dev coming onto an Ember project: "well, this new app uses MU, which is this whole set of rules, and this other app is still on pods, but not the original pods, the new colocated pods, and this addon is kinda like classic layout ...".

Not comprehensive

The Drawbacks section mentions that this RFC is exclusively focused on components, and that the narrow focus is justified by the relative prevalence of components and the fact that templates & JS files are coupled:

That, along with the fact that components are much more common, sets them apart from the rest and justifies solving the problem first.

I worry that this is the wrong framing. The potential drawback here is not which order to solve these in (i.e. components first vs. something else), but rather, solving them without considering the interdependencies. Module Unification, on the other hand, featured a thoroughly designed solution to filesystem layout that treated the project and the developer experience as a whole. I don't see a similar treatment of the broader problem in this RFC.

But I'm not suggesting that we need to go back to top-down, fully designed solutions. I'm a big fan of the direction that the template imports RFC goes: open up primitives that allow community experimentation, and once a stable, battle tested solution emerges, adopt it as the new standard. For the folks that like experimentation, they can play around on the bleeding edge with its associated papercuts. For folks wanting more stability, they can stick with tried and true patterns until the consensus emerges.

This pattern seems even more reasonable when we consider that we already support co-location: pods. The proposed design here certainly feels more ergonomic & performant on some margins, but Ember devs can already get most of the benefits of co-location by simply adopting pods.

This RFC unfortunately feels like an "all of the downsides, none of the upsides" compromise: we don't get the experimentation from the community, but we also don't get the comprehensive solution from top-down designs.

Delaying

We can forgo the opportunity to address these problems as part of Octane, and instead land this proposal (or something like it) after the edition has shipped. That would come at a cost of addon authors not being able to adopt some of the Octane features until we find alternative solutions.

This seems like a misunderstanding of editions (perhaps my own)? There's no hard feature lines with Octane - you can adopt the independent features piecemeal. Octane simply marks the "low point of incoherence". Waiting until after it "ships" simply means addon authors simply upgrading their Ember version like normal when the next feature lands.

@rwjblue

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

While I like the overall design proposed, there are some specific parts that seem problematic to me. The main thrust of my objection is that I believe that the template and component class are inextricably coupled (this RFC seems to agree), but if we accepted this RFC we are left in a fairly incoherent state from the perspective of component manager authors. Specifically, emberjs/rfcs#213 makes it very clear that a component manager should be the only source of information about a given component, but if implemented as proposed the template retrieval system is also making assumptions and retrieving information from the component class. This breaks the encapsulation that the manager interface is intended to provide.

I believe the correct change to the design here is to:

  • Expose a capability for providesTemplate (or similar but better name) to the component manager's optional features which allows managers to implement getTemplate (in line with the existing getContext hook)
  • Expose setTemplate / getTemplate utility functions to do the association (and assertions) proposed in this RFC so that arbitrary component manager authors can leverage them in their own getTemplate hooks
  • Use the setTemplate / getTemplate system by default when the providesTemplate optional feature is not enabled
@chriskrycho

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

Two high-level thoughts here:

  1. I share the concerns articulated by both @davewasmer and @rwjblue. Related, the similarity between this proposed structure and pods is unsurprising, but there is no discussion of the cost of a situation where users need to understand possibly all three mental models in the same project: it is often difficult to migrate everything at once. The discussion of the codemod (and indeed of the whole proposal) completely elides the existence of pods; and while pods are not a "first-class" feature in some ways, they are in widespread use and are supported output from Ember CLI.

  2. The combination of the way browser module imports work and the default (which I think is probably right) for how Node 12 supports ES Module imports (see discussion here) makes me wonder if simply using explicit imports (including extension names!) might be an important design consideration here. It's a difference from what Ember users do and expect today, but there are also a number of (potentially substantial) upsides to explicitly importing with file extension. Given as well the work toward static resolution with Embroider, I'd appreciate there being at minimum a discussion of those considerations here.

@simonihmig

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

I would second the concerns raised here, especially by @kovalchik and @davewasmer.

From what I understand this RFC proposes several (in my view different) things:

  1. associating a template at build time rather than resolving at runtime (performance)
  2. ability to extend a component including its template
  3. colocation

What's not obvious to me (and I might very well miss essential things here) is why these things are interdependent. Specifically couldn't there be ways to solve 1. and/or 2. without introducing a new file layout?

Regarding 3., I am certainly a big supporter for colocation. Fwiw, there was just one app we built in the past 5+ years that wasn't using pods. And that was using "MU" 🤷‍♂️😉

But I share the previously voiced concerns (mental churn, learning effort) that we are introducing yet another file layout. And this while we have an existing (pods) and a future (formerly known as MU) solution. And while those feel quite similar, the new proposal differs substantially IMHO: the collocated but flat layout is a new thing, and the nested one (awkwardly IMHO) introduces that index convention.

So assuming we do have to introduce a new (temporary) way of having colocated components to get the other benefits (see above), I would...

  • question why we need to actually introduce two new file layouts (flat and nested). Shouldn't we find agreement on one opinionated way? (hey, convention over configuration)
  • strongly opt for the nested version, as
    • it allows colocating other files easily and ergonomically as mentioned before (styles, tests)
    • prevents having an even longer list of files in your components/ folder
  • change the nested/folder version to the following layout, rather than introducing this index thing. Which is exactly what we had before with pods, and presumably (according to the MU RFC) what we get with the new file layout (ignoring the app/components/ base path)
├── components
│   └── foo-bar
|      ├── component.js
│      └── template.hbs

But at the end I would rather try to focus efforts on getting that thing formerly known as MU to land sooner than later, rather than binding resources (for implementation, documentation, codemods etc.) to a just temporarily used (AFAIU) new file layout.

@lifeart

This comment has been minimized.

Copy link

commented Apr 24, 2019

@rwjblue

  • Expose setTemplate / getTemplate utility functions to do the association (and assertions) proposed in this RFC so that arbitrary component manager authors can leverage them in their own getTemplate hooks
  • Use the setTemplate / getTemplate system by default when the providesTemplate optional feature is not enabled

Is it possible to get template asynchronously?

getTemplate -> promise -> someBinaryDataDownloaded from server / compiled in WebWorker .. etc

kgautreaux and others added some commits Apr 25, 2019

@joukevandermaas

This comment has been minimized.

Copy link

commented Apr 25, 2019

I don't really understand why Octane has to include file-system changes. I get that the Ember team is trying to ship a really nice new version of the framework that can be marketed and seems "modern", but imo Octane already does that. Just because module unification is now out of scope, does not mean it has to be replaced.

I think doing a last-minute change that has not gone through the same user-testing that the rest of Octane has, is more likely to hurt than to help. My feeling reading this RFC was that it will only confuse things more, because now there are three ways to lay out files, all of which have their own confusing inconsistencies. For example, I think routes and especially controllers are also "inextricably coupled" to their templates.

For what it's worth our app has used pods for a few years, and it has never really been a problem for us. If file-system changes are absolutely necessary, perhaps pods can be the default for Octane apps?

@rwjblue
Copy link
Member

left a comment

I reviewed this with @wycats and @chancancode, and now understand the reason for this being at a level above the component manager. With @chancancode's recent updates (making the API's of setComponentManager and setComponentTemplate much more aligned) I am 👍on this...

Show resolved Hide resolved text/0481-component-templates-co-location.md Outdated
Show resolved Hide resolved text/0481-component-templates-co-location.md Outdated
Show resolved Hide resolved text/0481-component-templates-co-location.md Outdated
@chriskrycho

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

@rwjblue @chancancode @wycats can one of you elaborate that for those of us who weren't able to be on that conversation? ❤️

@rwjblue

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

@chriskrycho - Sure, I can give it a shot, sorry if this turns out to be rambly.

My concerns centered around the conceptual encapsulation between the exported custom component (literally the export from the app/components/foo.js file) and the rest of the system. It was my position that nothing in the system should make assumptions about what that exported thing is and how to interact with it other than the component manager (note: I still largely agree with that position).

During the conversation I realized/discovered/whatever a few important points:

  • In order to support the "bundle compiler" mode (also know as AOT, "binary templates", and likely other names) we need to have a conceptually different "thing" handle compilation than the "browser runtime" component manager. In other words, there are essentially two component manager concepts: one compile time (where is this things template) and one for runtime concerns (give me this things context, update its arguments, etc).
  • The system in use by Ember (and soon to be migrated back into Glimmer.js) already requires that the manager is associated to the exported custom component (this is what setComponentManager does in fact), and therefore there is a required fundamental lookup just to get the manager.

Those two realizations (the first I just wasn't aware of, and the second one I knew but did think much about) made it clear that the proposal in this RFC for associating the template with the class is no different than the mechanism we already must use to associate the manager. Eventually, we may decide to merge the two things (e.g. 👋 👋 setComponentManager taking both a runtime and compile time factory 👋 👋), but right now the only thing needed from the compile time manager is "get this things template". In other words, setComponentTemplate is effectively the same thing as a theoretical setComponentCompileTimeManager (or setComponentManager(buildCompileManager, buildRuntimeManager, klass)).

The benefit of the existing proposal is that it lets move forward addressing the immediate concerns, and then we can iterate easily in the RFC that begins considering bringing "bundle compiler" into Ember.

Hope that mess of thoughts helped..

@wycats

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

In addition to an overwhelming amount of positive feedback, I'm seeing a few different sources of objections on this thread. I cite @davewasmer not to pick on him (his concerns are shared by several others), but rather because his articulation of the concerns is quite eloquent.

Ember devs can already get most of the benefits of co-location by simply adopting pods.

I understand the for a lot of Ember developers, pods "already exist", but I want to emphatically state that the details of pods are not the future of Ember, that there are many discrepancies between how pods work today and the form that any future direction for the framework will take, and that ecosystem tooling and library support for pods is spotty and inconsistent today.

Because pods are so popular, we absolutely will need a transition path from pods to the updated official file layout, of course.

The RFC readily admits this structure might change once again if / when some future form of Module Unification lands

Module Unification was the last feature that we attempted to land as a "big bang" update that came along with significant changes to the programming model. Features that we have attempted to land in this way have uniformly struggled to cross the finish line (routable components painfully comes to mind).

Going forward, I think it makes sense to improve the file layout as we identify standalone, useful improvements, rather than waiting for a wholesale update that will solve all of our problems.

This does not mean that each improvement will result in breaking changes, but it does mean that we might get stuck with the consequences of earlier decisions that we regret later on. In general, attempting to avoid regrets is a recipe for accomplishing nothing, and the long history of substantial Ember changes shows that Ember is no exception to this reality.

This also doesn't mean that we should make wild changes with nary a thought about what might come next. To the extent that we can reasonably predict our next technical steps, we should try to align our current work with those steps. To the extent that we can identify significant goals ahead of time, we should take steps along the way to increase our chance of accomplishing those goals.

But we shouldn't avoid making useful progress on the grounds that we can't perfectly prove that we won't regret the work we do today.

@GavinJoyce

This comment has been minimized.

Copy link
Member

commented Apr 27, 2019

I want to add some detail to why I'm keen on this RFC landing and to address some feedback from people who are already using pods and don't necessarily see the value in this.

As much as possible, we try to stay on the Ember happy path in Intercom. It's valuable to be able to avoid bikeshedding and short-cut technical discussions when there are clear conventions in place. The happy path helps onboard new engineers onto our product teams, the vast majority of people who work on our Ember app were new to Ember when they joined Intercom. We've been very successful sticking to Ember's happy path and we'll continue to follow it where possible.

While the improved developer ergonomics of the pods layout were always attractive, pods were never the Ember happy path. As long as I remember pods being available, I remember clear guidance from the core team that they do not represent the future. The guides have always reflected this too. For the last few years at least, a version of module unification was being actively worked on and constantly seemed just around the corner. We have been patiently waiting, and contributing where we can, for a better happy path file layout for years.

We obviously now have the benefit of hindsight, big projects often are a costly accumulation of hard work and assumptions and often fail without ever shipping. MU was one of those projects and we should learn from it: we need to improve Ember's file layout happy path incrementally.

This RFC is an incremental improvement to Ember's happy path file layout, it will deliver an immediate benefit to existing apps that don't use pods and to all new apps. It will likely be the first of many steps which will bring us to a comprehensive MU like file layout. By shipping this incremental change we will learn from real-world usage and future RFCs can leverage these learnings. Although these changes might not be directly applicable to apps using pods, they will ultimately benefit all apps as with this we move a step closer to having a single file layout for all Ember apps.

If you use pods, you can continue using them until a comprehensive MU like solution has landed. I strongly believe that that solution should be designed and shipped incrementally and that this RFC is a strong first step.

@simonihmig

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2019

Thanks @wycats for taking the time to go into details here and address the concerns. I think that was extremely helpful!

When a name.hbs is included alongside name.js, it works similarly to the TypeScript behavior when a name.d.ts is included alongside name.js. It's providing built-time information, but the .js file is the primary location of the imported module at runtime.

I feel the TS analogy is a bit misleading, as the typings file is just meta data, and cannot reasonably exist without the js file. While a template can exist without a component.js file and vice versa. So they are related, but different things.

But maybe that's really part of the (mental) problem I and others have, thinking of a component and its template as different things (types in MU nomenclature). Maybe the better mindset would be to think of them more as two sides of the same coin (even when one can optionally be omitted), which only when combined (compiled) together form that one thing called a component. This mindset would clearly demand not only to have those file co-located but also to give them the same name! (as they are just one thing, or MU type).

This RFC, therefore, diverges a bit semantically from pods. In the pods system, if you wanted to import component.js, you would have to say import './name/component', even though referring to the same component in a template does not include the filename component. This RFC aligns the name used to refer to a component in a template with its JavaScript module name, according to JavaScript idioms.

Very good point! Which made me reconsider my initial concerns, and which seemed to be missing in the RFC's motivation, or at least I didn't really get it before. I can see now how this can help pave the way for template imports...

Finally, I think it's important to mention that this RFC is not intended to be a stopgap that will be superseded by yet another tweaked file system layout some time in near future. Instead, it is an attempt to specify the long-term way in which component JavaScript files and their associated templates will be laid out next to each other on disk.

Ok, that seems to answer my other concern. So any future MU-successor RFC will take the precedent set by this RFC into account, and presumably not try to reinvent component file layout again (i.e. revert to component.js/ template.hbs), right?

guides, but this can be addressed in a future revision of the guides.

We will teach that the `templates` folder is used for route templates. This can
be introduced at the same time as the `routes` and `controllers` folder, which

This comment has been minimized.

Copy link
@jenweber

jenweber Apr 27, 2019

Contributor
Suggested change
be introduced at the same time as the `routes` and `controllers` folder, which
be introduced in the Guides at the same time as the `routes` and `controllers` topics, which

We will teach that the `templates` folder is used for route templates. This can
be introduced at the same time as the `routes` and `controllers` folder, which
would come at a later time than components.

This comment has been minimized.

Copy link
@jenweber

jenweber Apr 27, 2019

Contributor
Suggested change
would come at a later time than components.
come later in the Table of Contents.

## How we teach this

As mentioned above, the learning team will update the guides and API docs to

This comment has been minimized.

Copy link
@jenweber

jenweber Apr 27, 2019

Contributor

Suggested change to this first paragraph:

As mentioned above, we will update the learning resources to assume the "flat" co-located layout. Throughout the Ember Guides, the Tutorials, and the CLI Addon Tutorial, we would update all Component Handlebars code sample filenames. The prose describing the location of files would also need to change. The API documentation will describe the full set of file layout options supported in the major version of Ember. A section should also be added to the CLI guides, which describes how to properly import components from addons that have a mix of layout types. We will not cover blending file layouts in the Ember Guides, since they represent the happy path for an app, but we could link to the CLI Guides explanation.

This comment has been minimized.

Copy link
@chancancode

chancancode May 3, 2019

Author Member

👍 I applied the suggestions manually.

I continue to have doubts that it would be appropriate to cover these kind of things in the API docs, although I understand the motivation. Specifically, there is no natural place to cover abstract things like these in API docs – which must be attached to a concrete item, like a class or a function. Things like this (and angle/curly invocation styles, which was the last time it came up) are not a good fit for that because they apply to the abstract concept of components in general, not a specific class like Ember.Component (which is where they currently live as a compromise). I believe @wycats shares my concerns too.

However, we can stick with the current policy for the purpose of this RFC and discuss options outside of this thread.

@jessica-jordan

This comment has been minimized.

Copy link
Member

commented Apr 27, 2019

This reads great, thank you for writing up this RFC One thing I'm wondering about from a learning perspective is if there should be a commonly used term for the old location style of components. We could use that term to refer back to that pattern in e.g. the Guides or the Super Rentals tutorial where needed. I think it would also be helpful to have that common phrasing for not only the new location style (co-located components), but also for the old location style, to be used in learning resources like blog posts, talks or discussions by the community.

What are everyone's thoughts on this and if it makes sense to introduce a term for it, what could be a good name?

@rwjblue

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

Thank you @GavinJoyce, #481 (comment) very closely matches my perspective as well.

I wanted to take what @wycats said in #481 (comment) a bit further:

Finally, I think it's important to mention that this RFC is not intended to be a stopgap that will be superseded by yet another tweaked file system layout some time in near future. Instead, it is an attempt to specify the long-term way in which component JavaScript files and their associated templates will be laid out next to each other on disk.

It is my opinion (shared by many if not all core team members) that the original redesign for the filesystem in Ember apps (aka Module Unification / #143) is very very unlikely to actually land at this point. There are too many new factors that the original system doesn't account for (e.g. with template imports, why do we need local lookup?), @tomdale sums things up pretty well in this blog post. While I absolutely stand by the design work we did in #143 (and still quite like the final file layout that we landed on) we simply can't land it.

Instead of following through with the original MU design, we should instead shift our focus to making small iterative improvements to the file system layout. IMHO, these smallish bitesized improvements are the only way to actually ship a better layout. I see this RFC as the first in a series of RFCs, each to be digested on its own, but the culmination of which is a layout very much like the one proposed in #143.

tldr; Instead of attempting to implement all of the features of MU across the entire ecosystem at once, this RFC focuses on one aspect: component and template colocation. Let's get this done folks!

@Alonski

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

@jessica-jordan What about classic for old and Co located for this?

@Panman8201

This comment has been minimized.

Copy link

commented Apr 29, 2019

@jessica-jordan What about classic for old and Co located for this?

While I think "classic" is a good name for the original, "co-located" might be a bit too generic if there will be other incremental updates that do a similar thing (eg: controllers w/ their associated template). Instead, maybe be a bit more specific like "co-located components". Or perhaps those working on the file layout changes have a better suggestion based on other ideas for the incremental changes.

And/or, it might be easier to refer to it as something like "classic layout + co-located components". That way when the next increment update comes out you'll continue with that convention until we've reached "the new layout".

@rtablada

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

To start I'm very 👍 on moving to app/components with node style naming/resolution and even with shipping this as the blueprint default for Octane or soon after.

However, I don't share that thought on the technical implementations and other concerns raised (at best they deserve or need a separate RFC).

The other issues raised in the RFC are:

  1. extends bringing along templates from parent components (where applicable)
  2. Avoiding late template binding for the 80% use case for performance
  3. Adding public APIs for build time to associate templates with components (mostly to solve the above questions and give an implementation for how to support the new file layout without too many resolver or component manager changes)

These are both good goals and would fulfill a story for components but I think they are separate issues than colocation (though they do become more painful for sure).
My worry around these other two issues is that they seem to be in competition, conflict, or confusion with other efforts and RFCs (specifically #454).


To solve the main issue that this RFC puts forward (colocation of templates to match node naming conventions) this could be achieved with a broccoli build time move step that moves modules from the node path to the traditional path (with some consideration as well around import statements).

Is this ideal? No, but it doesn't muddy co-location and folder structure with other concerns.

In my mind this is a polyfill of sorts as the public primitives proposed like Embroider and/or #454 gain further consensus and implementation.
In that form as the primitives land, then the "mover" build step then gets replaced with a build step that uses APIs such as setComponentTemplate.

@oligriffiths

This comment has been minimized.

Copy link

commented Apr 30, 2019

This is excellent and I fully support colocation of component and template files.

A lot of the comments cover my overall concerns but I wanted to share some thoughts.

  1. If the intention of this RFC is to replace pods and iteratively improve upon file system layout in order to get towards MU (or its descendent architecture) then 👍. Can this be added in part of the preamble as to these intentions.

  2. Scope: This RFC seems to be covering multiple scopes:
    a. file system and developer ergonomics
    b. addon developer API
    c. runtime/compiler optimizations.
    d. proposing an official core-team approved co-location strategy (bye bye pods)

I would like the RFC to be very clear about exactly what the intentions are, and if at all possible, separate the scopes into separate complimentary RFCs.

  1. Pods: given that 2.a. is already technically possible with pods, explanation about why pods can't/shouldn't be used/improved to achieve the desired goal should be included. This should also be addressed in how we teach this, as there are valid concerns about a 3rd way of file system layout.

Over all this is awesome, and I look forward to seeing it progress. Thanks.

@PrinceCornNM

This comment has been minimized.

Copy link

commented Apr 30, 2019

We have been doing this with pods for years. It's a must for our team. It saves time and would benefit the whole community.

@Panman8201

This comment has been minimized.

Copy link

commented Apr 30, 2019

  1. Pods: given that 2.a. is already technically possible with pods, explanation about why pods can't/shouldn't be used/improved to achieve the desired goal should be included. This should also be addressed in how we teach this, as there are valid concerns about a 3rd way of file system layout.

"can't/shouldn't" is a bit strong since pods are used by a lot of projects (as proven by all the comments here). Instead of telling everyone to "switch back to classic layout", let the incremental improvements shake-out, at which point I think there will be a "natural" migration path and draw for projects to change. IMO, this RFC should have no impact on pods layout. Let pods be what it is, and continue to push classic layout forward.

@MelSumner

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

Another drawback is that it only address the co-location issue for components, not other related types like [[route, controller, route template]] and [[model, adapter, serializer]], or even co-location of tests. However, we believe the situation with components is unique enough (see the motivation section) that they are not merely related, but coupled. That, along with the fact that components are much more common, sets them apart from the rest and justifies solving the problem first.

While this is listed in the drawbacks section, I don't see this as a drawback. I think this is a benefit. I strongly agree with the justification that the component .js & .hbs files are coupled, not merely related- and I would be opposed to a solution that wanted to put the related files there too (i.e., tests).

I don't personally find it bothersome to navigate to the different folders (it never occurred to me that this was something I could be irritated about), it seems like a nice improvement and a salve for those who do find this DX troublesome.

For community members in this thread who have been mentioning or wondering about pods and/or module unification- please remember to read https://blog.emberjs.com/2019/03/11/update-on-module-unification-and-octane.html. While I think the position of the blog post solidifies that it's not part of Octane, it's also important for us all to understand that there ended up being significant issues with the approach overall, and it needed to be re-thought.

@oligriffiths

This comment has been minimized.

Copy link

commented Apr 30, 2019

@Panman8201 I only say that about pods to draw reasoning in the “alternatives” section as to why pods can’t be used for this, or why they shouldn’t be updated to support this use case, as opposed to outlawing their use. However as @wycats says, pods have never been an officially supported feature and it seems their inclusion flew in under the radar. They certainly don’t feature in the Ember “happy path”, however I personally love the collocation feature of components in pods. It may be worth exploring allowing pods to be enabled more granularly for specific things like components. Just a thought.

chancancode added some commits May 3, 2019

@chancancode

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

When a name.hbs is included alongside name.js, it works similarly to the TypeScript behavior when a name.d.ts is included alongside name.js. It's providing built-time information, but the .js file is the primary location of the imported module at runtime.

I feel the TS analogy is a bit misleading, as the typings file is just meta data, and cannot reasonably exist without the js file. While a template can exist without a component.js file and vice versa. So they are related, but different things.

I don't think this was intended to be a very important point, but you can definitely have a d.ts file without a corresponding js file.

Here is one example:

// enums.d.ts
export const enum Color {
  Red = "#ff0000",
  Green = "#00ff00",
  Blue = "#0000ff",
}

// index.ts
import { Color } from "./enums";

console.log(`Red: ${Color.Red}`);
console.log(`Green: ${Color.Green}`);
console.log(`Blue: ${Color.Blue}`);

// output
console.log("Red: " + "#ff0000" /* Red */);
console.log("Green: " + "#00ff00" /* Green */);
console.log("Blue: " + "#0000ff" /* Blue */);

But maybe that's really part of the (mental) problem I and others have, thinking of a component and its template as different things (types in MU nomenclature). Maybe the better mindset would be to think of them more as two sides of the same coin (even when one can optionally be omitted), which only when combined (compiled) together form that one thing called a component. This mindset would clearly demand not only to have those file co-located but also to give them the same name! (as they are just one thing, or MU type).

That's exactly right.

@chancancode

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

This reads great, thank you for writing up this RFC One thing I'm wondering about from a learning perspective is if there should be a commonly used term for the old location style of components. We could use that term to refer back to that pattern in e.g. the Guides or the Super Rentals tutorial where needed. I think it would also be helpful to have that common phrasing for not only the new location style (co-located components), but also for the old location style, to be used in learning resources like blog posts, talks or discussions by the community.

What are everyone's thoughts on this and if it makes sense to introduce a term for it, what could be a good name?

I think classic is fine for what we have now (matches the generator's --component-structure=classic), but I'm not sure if the "new" one needs a name more than we need a name for what we have today before this RFC.

That is to say, when the guides are updated, what is proposed here would just implicitly be the "current/recommended/default" file layout.

@chancancode

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

@rtablada @oligriffiths This scope/intention of this RFC is to presents a solution that solves all four problems listed in the motivation section. I don't think it would make sense to split it up. Co-location is a solution, not a problem – a solution to the four problems listed in the motivation section. Of course, we could split up the four problems into different RFCs, but it would then make sense to merge them since they have the same proposed solution. Do you have any concrete objections to the proposed solutions? If so, let's discuss the concrete objections.

This RFC does not intend to conflict, replace or otherwise interfere with other RFCs like #454 (or "strict mode", etc). They will all be built on top of the setComponentTemplate primitive here. If there are any specific concerns let's discuss the concrete issues.

@chriskrycho

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

(Minor aside as a point of clarification for any ember-cli-typescript v2 users: the example @chancancode shared about const enum types is perfectly valid TypeScript, but doesn't work in ember-cli-typescript b/c we use Babel for transpiling TS and it doesn't support const enum. The point stands in general: you can have type-only exports in .d.ts files and those are fine. The example provided works in regular TS because a const enum is a type-only export; a non-const enum would not work because it actually generates runtime code.)

@MelSumner

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

We have been doing this with pods for years. It's a must for our team. It saves time and would benefit the whole community.

@PrinceCornNM - can you clarify? Are you saying that you agree with this RFC, because the component js/hbs co-location is similar to pods? Or something else?

@rwjblue

This comment has been minimized.

Copy link
Member

commented May 6, 2019

We discussed this at the Ember core team meeting this past Friday, and are moving this into the final comment period.

@Panman8201 Panman8201 referenced this pull request May 6, 2019

Open

Octane Tracking Issue #17234

78 of 138 tasks complete
@davewasmer

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

Instead of following through with the original MU design, we should instead shift our focus to making small iterative improvements to the file system layout. IMHO, these smallish bitesized improvements are the only way to actually ship a better layout - @rwjblue

This is the core of my concern with this RFC. I'm strongly in favor of co-location overall, and have no opinion on the implementation details described here. But I'm worried that this approach to incrementally tweaking the filesystem layout will lead to confusion, and ultimately, end up thwarting a core benefit of Ember:

As much as possible, we try to stay on the Ember happy path in Intercom. It's valuable to be able to avoid bikeshedding and short-cut technical discussions when there are clear conventions in place. - @GavinJoyce


Let's assume this RFC lands, and eventually a couple more tweaks to the filesystem layout land as well (let's say route folders and ... something with helpers - I'm just making this up as an example). When a new dev joins an Ember team and starts to work on their app, how will they know where to find things on disk?

So far, the answer has been: "pods or classic".

With this incremental approach, it seems like the answer will be: "well, we use Ember's classic structure, except we use the co-located component templates and the new route folders, but not the new helper layout".

Each tweak would seem to introduce a new mental model that must be layered on top of all the previous mental models to produce the filesystem structure for a given app. For something so basic and core to the experience of developing an app as "where does the code I care about live?", that's a lot of cognitive load.

Finally, I think it's important to mention that this RFC is not intended to be a stopgap that will be superseded by yet another tweaked file system layout some time in near future. ... This RFC is saying that when we eventually tackle route layout, to finish the job of standardizing the features in today's pods layout, they should be name.{js,hbs}. - @wycats

My worry is that this is not the way Ember users will experience this RFC. Yes, on some level, the RFC is aimed at uniting the component's JS and template (a worthy goal). But in practical terms, the way it changes dev experience is a new filesystem layout model (now a 3rd, after classic and pods).

When, say, a route layout RFC lands as well - that's another (4th!) layout model. I don't think we can treat this RFC as a different level of abstraction that just combines nicely with some future route layout RFC - the practical effect is that a route layout RFC adds another layer of rules to be mentally evaluated with all other previous sets of rules.

The end result of this RFC plus a routing layout RFC would be 4 different ways to structure an Ember app. To make it even worse, there will likely be some mixture of them allowed to coexist in the same app at the same time. That seems like a significant mental burden.


I'm curious why we can't expose lower level primitives that allow this to be experimented with in addon-land, like the template imports RFC, as I suggested above. I'll admit I haven't fully grokked the underlying implementation details outlined here so far - is that fundamentally not possible, or simply not a goal?

@wycats

This comment has been minimized.

Copy link
Member

commented May 7, 2019

Let's assume this RFC lands, and eventually a couple more tweaks to the filesystem layout land as well (let's say route folders and ... something with helpers - I'm just making this up as an example). When a new dev joins an Ember team and starts to work on their app, how will they know where to find things on disk?

So far, the answer has been: "pods or classic".

I think this is a little bit misleading. At minimum, the current answer is pods, classic or mixed.

image

As much as people are convinced that they love Pods and we should just have adopted them wholesale, Pods were always an experimental approach with a number of edge-cases. It's hard to catalog the edge-cases precisely because Pods was never incorporated into Ember proper.

We aren't going to eliminate Pods over night because, like or not, Pods were included as an option in the Ember blueprints, and at various points were recommended strongly by important members of the Ember ecosystem. But they were never a consensus position, had known edge-cases that many people don't understand, and interact poorly with any number of addons.

The goal of this RFC is to migrate us, over the next year or so (my guess) to a single answer to the question. A single answer is better than two answers, but it does require us to have three answers in the meantime. We're hopeful that strong codemods will reduce the impact of the change, and don't intend to force people to schedule their teams to migrate to co-location any time soon (certainly not before 4.0).

What's missing in this analysis (both the original comment and my response so far) is the question a developer asks about a new app: "which file layout should I use". Today, the answer is: "the default, but maybe pods" (depending on who you're talking to). The goal of this RFC is to significantly increase the number of people for whom the right advice will be: "the default, duh". We believe that since co-location addresses a significant enough part of the reason people advocate for pods, and has the benefit of being built-in, that more people will advocate for the default layout.

This is speculation, of course, but so is everything else we've been talking about. I don't know for sure that this speculation is right, but getting to the point where Ember has one default file layout that is supported by the vast majority of the community is an extremely important goal that I think needs to take precedence here.

@davewasmer

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

getting to the point where Ember has one default file layout that is supported by the vast majority of the community is an extremely important goal that I think needs to take precedence here.

I agree wholeheartedly 💯

I think this is a little bit misleading. At minimum, the current answer is pods, classic or mixed.

Fair point. Allow me to rephrase my objection below - I think @wycats response here has helped me clarify my own thinking on this.

As much as people are convinced that they love Pods

I should perhaps clarify - I'm not suggesting we adopt pods wholesale as the solution. I'm personally in favor of some kind of co-location, but fully acknowledge the shortcomings of pods. I'd love a fully support co-located filesystem layout eventually.

A single answer is better than two answers, but it does require us to have three answers in the meantime.

But it seems like (and perhaps I'm misunderstanding here?) that's not the end of the story. We'll have 3 answers once this RFC lands. Then 4 if/when a routing layout RFC lands. Possibly more if we do more tweaking beyond that.

In the long run, sure, we'll settle on one solution, but I worry that the multiplicity of solutions in the meantime is unnecessarily painful.

I've asked about an alternative approach a couple times so far, but didn't see anyone address it yet. I wonder if we could try to expose lower level primitives that allow addon-land code to experiment. Once the community begins to find consensus on a holistic solution, we RFC that for core adoption.

I'm sure such an approach has already been considered, so I'd love to understand what its downsides might be.

@GavinJoyce

This comment has been minimized.

Copy link
Member

commented May 7, 2019

We'll have 3 answers once this RFC lands. Then 4 if/when a routing layout RFC lands. Possibly more if we do more tweaking beyond that. (@davewasmer)

In the long run, sure, we'll settle on one solution, but I worry that the multiplicity of solutions in the meantime is unnecessarily painful. (@davewasmer)

I don't view these incremental steps as necessarily introducing new solutions nor that they will be particularly painful. The rules for where files live in any particular app at any one time aren't that complicated and I don't believe that the rules changing, either incrementally or all at once, will be any more of a burden than the usual set of incremental changes that upgrading an Ember app can bring.

As I see it, there are three scenarios for apps during this incremental and transitional phase:

Apps using pods:

During the transitional phase: Do nothing, continue to use pods.
Once complete: Use provided codemods to migrate away from pods to new happy path.

Apps using the current file layout and wishing to migrate incrementally:

During the transitional phase: Migrate step by step using provided codemods.
Once complete: Nothing to do, already using new happy path.

Apps using the current file layout and wishing to wait until the new happy path lands:

During the transitional phase: Do nothing, continue to use their existing file layout
Once complete: Use provided codemods to migrate to new happy path

@GavinJoyce

This comment has been minimized.

Copy link
Member

commented May 7, 2019

In addition to my reasons why I'm excited for this RFC in Intercom, I'm also keen as I believe that it will significantly improve our chances of winning the next generation of future Ember developers.

I'm currently working on what will hopefully be a high quality and highly polished comprehensive Ember tutorial for the freeCodeCamp youtube channel (~1 Million subscribers !!).

Octane brings an opportunity like we've never had before to win the hearts and minds of the next generation of web app developers and ambitious companies. One of the first things I usually do in tutorials like this is to create some components. Glimmer components in Octane are an early opportunity to show why modern Ember is exciting, elegant and in many ways better than other, currently more popular alternatives. Having to explain why the component javascript and template are located in two different places immediately puts Ember at a significant disadvantage to other competing frameworks like React though. For this reason, I've always have this little internal battle over whether I should use pods or not when planning a new tutorial. I've never ended up using pods in tutorials as they themselves require some explanation at the beginning, but I know that EmberMap sometimes do use pods for tutorials.

Octane is a significant milestone for Ember, we'll likely have a moment of significant interest from the wider Javascript community when it's available. If this RFC lands in a similar timeframe to Octane, we'll be able to create much more polished and concise tutorials that will maximise the chances that new people will consider investing more time in exploring, learning and adopting Ember.

@MelSumner MelSumner removed the Octane label May 8, 2019

@michaelrkn michaelrkn referenced this pull request May 12, 2019

Closed

Controllers and Octane #488

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.