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

Module Unification #143

Merged
merged 5 commits into from
Oct 18, 2016
Merged

Module Unification #143

merged 5 commits into from
Oct 18, 2016

Conversation

dgeb
Copy link
Member

@dgeb dgeb commented May 9, 2016

@dgeb dgeb mentioned this pull request May 9, 2016
@rwjblue
Copy link
Member

rwjblue commented May 9, 2016

@dgeb - Thank you very much for iterating on this!

I am very much in favor of this proposal, I have only one or two relatively small concerns that I will try to comment on later tonight.

@dgeb
Copy link
Member Author

dgeb commented May 9, 2016

@rwjblue - Thanks as well to you for all your help - your migration tool has proven invaluable. Looking forward to hearing any remaining concerns.

@ebryn
Copy link
Member

ebryn commented May 9, 2016

I don't think I could be any happier about how this RFC has turned out. Our iterations have paid us back in dividends! 💰

@willmanduffy
Copy link

Overall I think this is super cool and I can tell from just my first look into this that there was a lot of thought put into it. So for starters, thanks for your hard work!

Couple things I wanted to ask about:

Avoid the "titlebar problem", in which many files are all named "component.js" and you can't tell them apart in your editor.

You list this as a design constraint, but does the RFC address this? For example I cloned down https://github.com/rwjblue/--ghost-modules-sample/tree/grouped-collections/src and opened up 10 files (a common use case for me) and suddenly...

screen shot 2016-05-09 at 2 56 28 pm

Secondly I have a mild concern about naming the folder models. For a beginner user terminology can be pretty confusing. To a beginner user this reads that adapter.js, serializer.js, and model.js are all types of models but it's not clear what differentiates model.js from any other model. Why is the same terminology used? What differentiates it? Just feels a little like muddying the waters.

Not a deal breaker at all, just something I wanted to raise.

@rwjblue
Copy link
Member

rwjblue commented May 9, 2016

Avoid the "titlebar problem", in which many files are all named "component.js" and you can't tell them apart in your editor.

You list this as a design constraint, but does the RFC address this?

Yes. Take the following example:

src/
  ui/
    components/
      foo-bar.hbs // a template only component
      baz-qux.js // a JS only component
    routes/
      posts.js // a post route

In these cases, there are only a single file and the file can be named after its type. In today's pods you would have:

app/
  foo-bar/
    template.hbs
  baz-qux/
    component.js
  posts/
    route.js

Extrapolating that out, would result in many many files named component.js, template.hbs, and route.js. In the new system we would eliminate quite a few of these generically named files, and only end up creating a directory if you actually do end up needing multiple files.

Now, as you have noticed it is not guaranteed to solve the full title bar issue (because there will likely still be many files named component.js), but it definitely is an improvement over the current pods structure.

template.hbs
paginator-control
component.js
template.js
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should likely be .hbs instead of .js

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@btecu - good eye - thanks!

@willmanduffy
Copy link

willmanduffy commented May 9, 2016

No argument here that this definitely improves a lot over the current pods structure, but alternately doesn't it make it worse for people migrating from a non-pods app (like me)? Suddenly I'm going from.

app/
  components/
    foo-bar.js
    baz-biz.js
  templates/
    components/
      foo-bar.hbs
      baz-biz.hbs

to

src/
  ui/
    components/
      foo-bar/
        component.js
        component.hbs
      baz-biz/
        component.js
        component.hbs

I can definitely see this making the titlebar problem worse for some. I should note that I probably have a pair of templates / js for maybe 70% of my components but this is anecdotal data tied to my coding style obviously. No info on how others work.

@landongn
Copy link

landongn commented May 9, 2016

The only thing that seemed unclear to me was the designation of data or ui as the root folders. Are those arbitrary, and can they be any rational word? (since $src would allow the parent container folder for these items.)

Otherwise this looks awesome! I'm a big fan of pods, but this just making the pod structure more organized, which is a growth pain that many medium-to-large projects will face when using pods.

@Gaurav0
Copy link
Contributor

Gaurav0 commented May 9, 2016

The main module of an addon can also declare a rootName, which is used by the resolver to lookup main modules. Initially, the rootName will be a read-only property that equals the modulePrefix with any ember- and ember-cli- prefixes stripped (e.g. ember-power-select becomes power-select). It's possible that we may allow overrides / aliases in the future.

I'm concerned if this might create backward incompatibilities if we don't allow overrides / aliases. For example, if ember-power-select went over to this new format, apps currently using it via import PowerSelectComponent from "ember-power-select/components/power-select-multiple"; could no longer import it that way and would need to use import PowerSelectComponent from "power-select/components/power-select-multiple";

@rwjblue
Copy link
Member

rwjblue commented May 9, 2016

@Gaurav0:

I'm concerned if this might create backward incompatibilities if we don't allow overrides / aliases. For example, if ember-power-select went over to this new format, apps currently using it via import PowerSelectComponent from "ember-power-select/components/power-select-multiple"; could no longer import it that way and would need to use import PowerSelectComponent from "power-select/components/power-select-multiple";

The modules on disk will not be changing. The rootName is used for things that the resolver is currently responsible for. In the ember-power-select case that is the template invocation of {{power-select}}. Existing imports would largely be unaffected.

@dgeb dgeb force-pushed the module-unification branch from 18df372 to 220516f Compare May 9, 2016 19:52
@dgeb dgeb force-pushed the module-unification branch from 220516f to fca9184 Compare May 9, 2016 19:54
@dgeb
Copy link
Member Author

dgeb commented May 9, 2016

The only thing that seemed unclear to me was the designation of data or ui as the root folders. Are those arbitrary, and can they be any rational word? (since $src would allow the parent container folder for these items.)

@landongn data and ui are considered "collection groups", which are explained in more detail here. Any addon will be able to introduce its own types, collections, and groups. The ui group will be a default in ember-cli, while data will be declared and introduced by ember-data (or other data libs).

@dgeb
Copy link
Member Author

dgeb commented May 9, 2016

Secondly I have a mild concern about naming the folder models. For a beginner user terminology can be pretty confusing. To a beginner user this reads that adapter.js, serializer.js, and model.js are all types of models but it's not clear what differentiates model.js from any other model. Why is the same terminology used? What differentiates it? Just feels a little like muddying the waters.

@willmanduffy I've tried to explain this concept in the teaching section for collections and types. Please let me know if this clarifies the waters at all or if I still have work to do.

@rwjblue
Copy link
Member

rwjblue commented May 9, 2016

@willmanduffy:

No argument here that this definitely improves a lot over the current pods structure, but alternately doesn't it make it worse for people migrating from a non-pods app (like me)?

Agreed, but many other parts of this structure are quite a bit more ergonomic. For example, when working deeply inside a specific route structure in app/routes/admin/posts/post/comments/index.js and need to navigate to that routes template you have to traverse quite a ways to app/templates/admin/posts/post/comments/index.js.

Also, even in classic structure the "titlebar problem" can happen. You may have foo-bar.js and foo-bar.hbs open (and the extension often is truncated when a large number of tabs are open); foo-bar.js (route) and foo-bar.js (component); I am sure there are many more scenarios that would cause issues also.

I personally think that I would be fine with removing that from the list of constraints. I agree with you that we haven't fully addressed it, but I believe that the consensus on the core team is that we are satisfied with that aspect of the proposal.

@mmun
Copy link
Member

mmun commented May 9, 2016

Editor plugins like https://atom.io/packages/ember-tabs go a long way to making the titlebar problem moot.

@tomdale
Copy link
Member

tomdale commented May 9, 2016

@willmanduffy @rwjblue I agree that it seems like the RFC in its current form doesn't address this specific problem. I'm beginning to think that perhaps the best path forward is to identify the top N popular editors in the community, and recommend to users that they adjust their configuration accordingly to better display a relevant filename. Additionally, many editors allow you to drop a configuration file in the root of a project directory—we should explore whether there are better out-of-the-box configurations Ember CLI could generate for new projects.

@landongn
Copy link

landongn commented May 9, 2016

data and ui are considered "collection groups", which are explained in more detail here. Any addon will be able to introduce its own types, collections, and groups. The ui group will be a default in ember-cli, while data will be declared and introduced by ember-data (or other data libs).

This makes sense, though I'm struggling slightly with the name of 'collection'. The purpose of the ui folder seems like it's declaring a logical group, rather than a collection (which has semantics to programmers outside of organization, such as sets, arrays, and so forth).

A few of the terms took multiple parsing before I understood exactly what was happening. util in particular has a very broad use case and since it can contain any other type within it, it could turn into a 'place to put stuff' when the developer is out of other options that conflict with their understanding. If the ui module is where the lions share of work is done, then that makes sense. If there are components within the data folder, I feel like this is a concern worth limiting. It seems odd that we would need a ui folder if components can potentially live under any collection/ folder.

Am I parsing too heavily on the semantics side for this to be relevant? Probably, though it's worth considering if the muddying of the waters with regards to separation of concerns is worth discussing here for this RFC.

@rwjblue
Copy link
Member

rwjblue commented May 9, 2016

@landongn:

It seems odd that we would need a ui folder if components can potentially live under any collection/ folder

I completely agree, that would be odd indeed. This proposal limits the locations for components to src/ui/components and private collections within src/ui/routes/**/-components. That is discussed in the Module Collections section. You can see that the components type is only allowed in its own collection or as a private collection inside of the src/ui/routes collection.

@landongn
Copy link

landongn commented May 9, 2016

@rwjblue my mistake, that was not clear to me. Thanks for clarifying!

edit err...

The following private collections are allowed within collections (rule 5):
components - utils

Is private really the correct distinction here?

edit edit: is there anything preventing me from importing a component from util into a src/ui/component as a subclassing exercise? is that a potential goal or just something that's technically allowed but discouraged ? If it's the latter, it doesn't seem like there's any indication if these are good ideas or not, but seem like they could be rife for abuse from new and learning developers.


Every resolvable module must have both a `name` and a `type`. The `type`
typically corresponds to the base class of the module's default export (e.g.
`route`, `template`, etc.).
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this work if I'd like to export a singleton that's not a service or a discrete ember subclass? This phrasing implies that there is a requirement for a module to be a specific and explicit type, though I don't think that's the intention.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@landongn The utils collection is intended for general ES modules. Its contents are completely ignored by the ember resolver. The name and type requirement is specific to resolvable modules.

Copy link
Contributor

@dfreeman dfreeman May 9, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That distinction threw me a bit in the listing of what private collections are allowed where.
Since the contents of utils aren't intended to be resolvable anyway, is it necessary to declare that utils is an allowed private collection everywhere?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dfreeman utils is allowed as a private collection to enable co-location of lib files wherever they are needed. This makes relative ES module imports cleaner, and makes for easier movement of whole "branches" of functionality.

The - prefix signals to the resolver that a directory is a private collection and has its own rules distinct from the collection that contains it. If you did not place local lib files in -utils, the resolver would treat them as members of the parent collection and try to interpret their name, type, etc.

@dgeb
Copy link
Member Author

dgeb commented May 9, 2016

is there anything preventing me from importing a component from util into a src/ui/component as a subclassing exercise? is that a potential goal or just something that's technically allowed but discouraged ? If it's the latter, it doesn't seem like there's any indication if these are good ideas or not, but seem like they could be rife for abuse from new and learning developers.

@landongn Perhaps the key point here is that the Ember resolver will use the conventions defined in this RFC to understand where to find things. If you don't care if your base component is accessible from the resolver, you could place it anywhere and import it into your subclass. That is all happening at the ES module dependency level. However, I wouldn't recommend doing this because chances are that you will also want to to co-locate tests for your base component, and the test runner will also use a resolver to find tests in collections. Incentives like this should be strong encouragement for developers to follow conventions.

@mitchlloyd
Copy link

I'm trying to understand the need for - and a designation for private collections.

As mentioned earlier, utils are not relevant because Ember doesn't resolve utils. Now that there is no way to invoke nested component (e.g. {{my/nested/hyphen-component}}) it appears that any component outside of the top-level src/ui/components directory cannot be invoked globally.

Is the intent here to prevent naming collisions in case someone wants a route named components? Are there other private collections we might see in the future?

@tamebadger
Copy link

tamebadger commented Oct 10, 2016

Please let's not. The whole point of Ember is strong conventions allow us to have less configuration, less boilerplate, etc. A lot of bugs in addons are for not supporting alternative configuration options. For example, fastboot doesn't work right now if a rootElement other than "body" is specified. It is so much easier to develop addons that work for everyone if everyone has the same configuration. So the fewer options we have the better.

While it would be nice to make everyone happy, let's not do so by compromising what makes Ember special.

@Gaurav0
Don't you think it's worth investigating where Ember should/could be flexible ? Think about the wide use of linting tools and how everyone sets the configuration for themselves, and how they can adopt and "extend" Ember's ESLint rules for example ?

@Gaurav0
Copy link
Contributor

Gaurav0 commented Oct 10, 2016

@Gaurav0
Don't you think it's worth investigating where Ember should/could be flexible ? Think about the wide use of linting tools and how everyone sets the configuration for themselves, and how they can adopt and "extend" Ember's ESLint rules for example ?

Ember CLI's addon system allows for a great deal of flexibility. Some people are experimenting with ESLint while I and most of the Ember ecosystem continue to use the jshint based linting currently included by default in the stable version of ember-cli. When it is determined by consensus what the best ESLint rules are for Ember, we will hopefully all coalesce around a single shared configuration, just as we are in the process of doing here for file structure. This will best allow us to all use the best practices as discovered by the community, while continuing to have all the advantages of having us all do the same thing. One such advantage is, as mentioned in this RFC, being able to write an addon that upgrades source code for us to the new directory structure we've decided on.

No matter what file structure is decided on in this RFC, you will be able to change it for your project by writing an addon. But I hope you will realize that sharing the solution agreed upon by others is a better choice.

@tamebadger
Copy link

That sounds awesome! I agree a 100 percent on:

But I hope you will realize that sharing the solution agreed upon by others is a better choice.

I'll add to this that, I don't think it's worth trying to convince everyone there is one solution that will suit everyone. It's a better approach (IMO) to say, we have primitives available to allow people to create solutions that are shareable, with a main solution as the "Ember" solution.

And that's basically what you are also saying with:

No matter what file structure is decided on in this RFC, you will be able to change it for your project by writing an addon.

So I'm happy if that's the case.

@trek
Copy link
Member

trek commented Oct 10, 2016

Don't you think it's worth investigating where Ember should/could be flexible ?

and

It's a better approach (IMO) to say, we have primitives available to allow people to create solutions that are shareable, with a main solution as the "Ember" solution.

We actually do have this today and retain it with the approach being discussed in this RFC. Under discussion is the default behavior of the resolver. But anyone can choose to supply their own custom resolver that augments this default behavior or, if you wish, entirely replace it with a resolver that finds modules dynamically based on your preferences.

In that sense @rtm's desire already exists:

Instead of proposing yet another "father knows best" standard project structure, our focus should be on how to flexibly support whatever structure the developer chooses. Yes, that might involve revisiting how the resolver works or even the basic concept of the resolver.

We wouldn't need to revisit the concept of the resolver because this a point of the resolver.

That said: many specific but arbitrary project structures places a slight burden on developers moving from project to project. And, as @Gaurav0 so eloquently notes, this places a high burden on addon authors.

So, if you opt into a entirely custom resolver it's unfair to expect addons to work bug-free in your custom app or to expect addon authors to make their addon work with your app.

A world where an application developer needs to expend effort to make addons correctly interact with their application is the world everyone using Ember is explicitly opting out of.

@grapho
Copy link

grapho commented Oct 11, 2016

I will just put in my two cents: I have been at Ember for a while now.. and have seen many changes.. and many breaking changes as well.

Though I sympathize with concerns (especially those of beginners and outsiders) that pushing people into a new organizational structure will case some upset. At the end of the day, Ember is a framework and well, sometimes you need to change the frame to allow bigger and better things.

All I personally care about is if I use the framework, will my life be easier, or will it be harder? If I just follow the new rules of the framework.. will I be able to make Apps better, faster, stronger?

I believe that is going to be a vast improvement and I cannot wait for this to land.. just the module structure.. for now, I don't even know yet how excited I am about the things that will follow Module Unification.. all I know is that the classic app structure is outdated and inefficient for a large app.. and Pods does not solve my issues.

Anyway, good work, that is my final comment :)

@tamebadger
Copy link

I'm with you @grapho ! Really excited for this, it's already going to be massive improvement and is indeed necessary.

@trek, on the following:

So, if you opt into a entirely custom resolver it's unfair to expect addons to work bug-free in your custom app or to expect addon authors to make their addon work with your app.

I'm just wondering if there is a possibility for resolvers to all output some structure that addon authors can count on..like a resolved-map structure that others can create regardless of file and directory naming and structure. This is probably over simplifying it, and more investigation necessary. Any recommended code to look at will be appreciated, I will start with https://github.com/ember-cli/ember-resolver.

@dgeb
Copy link
Member Author

dgeb commented Oct 12, 2016

I'd like to unpack the philosophy behind the organizational structure more because I think it will go a long way towards alleviating concerns about "too much magic" through conventions and give a glimpse into some options for ember modules in the future.

Let's start by saying that every default type in a collection (e.g. components, models, routes, etc.) may have a default representation plus named aspects that represent different facets of it. For example, a "component" can have a default representation that is a Component subclass, but can also have related aspects such as a template, a component-test, and (coming soon) even a style.

This concept of a default representation plus named aspects aligns well with ES6 modules, which can have both default and named exports.

This RFC carries this parallel further into the file layout, which allows two forms of naming in collections:

src/${collection}/${namespace}/${name}
src/${collection}/${namespace}/${name}/${type}

Let's call the first form "compact" and the second "exploded". The compact form bundles together all the related aspects of a "thing" (i.e. component, model, etc.) into a single file with named and/or a default exports. The exploded form spreads the named aspects into separate files in a subdirectory.

Let's take a look at a simple date-picker component in both forms.

Here's the exploded form, spread across three files:

// src/ui/components/date-picker/component.js
import { Component } from 'ember';

export default Component.extend({
});

// src/ui/components/date-picker/template.hbs
<div class="date-picker"></div>

// src/ui/components/date-picker/style.css
.date-picker { color: red }

This is equivalent to the compact form, all in one file:

// src/ui/components/date-picker.js
import { Component } from 'ember';

export default Component.extend({
});

export const template = hbs`
  <div class="date-picker"></div>
`;

export const style = `
  .date-picker { color: red }
`;

Note that we're not actually recommending the compact form now because of the need for:

  • Improved ergonomics of embedded templates in JS.
  • Scoping of styles to components.

Expect future RFCs for both.

The purpose of this example is to point out how the compact and exploded forms are compatible and complementary. You could define your default component export in src/ui/components/date-picker.js and then supplement it with a template contained in src/ui/components/date-picker/template.hbs. This would be equivalent to the following explicit wiring:

// src/ui/components/date-picker.js
import { Component } from 'ember';
export { default as template } from './date-picker/template.hbs';

export default Component.extend({
});

// src/ui/components/date-picker/template.hbs
<div class="date-picker"></div>

All the conventions are doing is eliminating the need for the above re-export. Because this RFC's layout and ES6 modules themselves are all statically analyzable, we have the opportunity to do this kind of merging of modules at build time to minimize the number of modules actually used and thus improve run-time performance.

We are not actually taking away the right or ability to do this kind of wiring on your own. We are simply eliminating the need to do so when you use the default resolver, which will be a big plus for developer ergonomics.

One other thing to note is that modules in their compact form may go a long way to reducing heartburn over file-name repetition. A component can be exported from src/ui/components/date-picker.js OR src/ui/components/date-picker/component.js. However, a build-time error will result if both are present and have a default export.

I hope that this expansion of the philosophy behind this RFC relieves some concerns. I had initial concerns about over-complicating an already long RFC by unpacking this, but the subsequent discussion has made me realize it's important to share.

@tomdale
Copy link
Member

tomdale commented Oct 18, 2016

My thanks to everyone who has contributed to this RFC thread. A proposal that touches one of the most fundamental aspects of an application—how its file structure is laid out—is one that is important to get right. We can't do that without feedback from as wide an audience as possible.

That this discussion was so spirited, respectful, detail-oriented and productive (given the internet's tendency to be otherwise) makes me incredibly proud to be part of this community, and I think shows the value of our RFC system.

After two Final Comment Periods (FCPs), the core team feels confident that any glaring problems have been identified and we can now begin working towards implementation of the RFC.

The biggest tradeoff of the current proposal is editor ambiguity caused by identically-named files. We've heard those concerns loud and clear. The goal of an RFC is not to achieve perfection out of the gate. In that case, RFCs would take years, if they ever shipped at all. Rather, RFCs should provide a clear, incremental step forward, while still leaving us the opportunity to address in the future any issues that were identified.

In this case, we feel confident that the editor confusion issue can be addressed in one or more of the following ways:

  • Configuring editors to disambiguate identically-named files
  • A future RFC to support named exports
  • Providing clear guidance on customizing the resolver rules for your application. (Ember's conventions should always be considered guidelines, not edicts. We should always have a way to override conventions that don't work for a particular app.)
  • If none of the above solutions are found to be sufficient, nothing in this RFC blocks a future RFC that could propose an optional annotation to file names to include both name and type information.

I'd like to thank @dgeb for his hard work and diligence over the more than six months he's been working on this. This proposal unlocks many different performance optimizations that will be critical for improving the load time of mobile apps, as well as providing a more consistent experience for developers that's in line with Ember's principle of Convention over Configuration. I'm excited to use the new file structure in my own apps.

With that, I'm happy to
x9znskm

@webark
Copy link

webark commented Oct 20, 2016

I know this is closed, but just a question on where mixins will got with this? Or are you planning on phasing those out.

@locks
Copy link
Contributor

locks commented Oct 20, 2016

@webark mixins are not part of the resolver story, to use them you have to import … as a regular ES6 module. A possible solution is having a top-level collection that's ignored by the resolver, or putting them in /utils. I suggest keeping an eye open for when the blueprints get updated, as this discussion will need to be had to update the mixin blueprint!

@webark
Copy link

webark commented Oct 21, 2016

@locks ... 🐑 ... that was an obvious answer!! Yes mixins are imported and you don't need to resolve them! Thanks for answering my, albeit somewhat silly, question.

@paulyoder
Copy link

FYI: here's the issue in Ember CLI's repo that's tracking the progress of this feature.

@bartocc
Copy link

bartocc commented Mar 29, 2017

Here's another issue in the emberjs repo tracking this
emberjs/ember.js#14882

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.