-
-
Notifications
You must be signed in to change notification settings - Fork 406
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RFC: JavaScript Modules API #176
Conversation
Into this: | ||
|
||
```js | ||
import Component from "ember-component"; |
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.
Should this be import Component from "@ember/component";
?
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.
@trek Nice catch, thank you!
|
||
```js | ||
import EmberObject from "@ember/object"; | ||
import get from "@ember/metal/get"; |
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.
wasn't the shims syntax:
import get from 'ember-metal/get';
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.
Whoops, overzealous regexp on my part. Thanks for catching that!
|
||
#### Guiding Learners | ||
|
||
We can group framework classes and utilities by functionality, making it clear |
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.
🎉
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 huge, framework discovery through intuition is a huge part of how devs learn. This also has the side benefit of making the documentation much easier to find.
|
||
### Use Scoped Packages | ||
|
||
Last year, [npm introduced support for scoped packages][scoped-packages]. Scopes |
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.
Do npm's scoped packages play nicely with yarn?
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.
Hm, I've had success with yarn and non-private scoped packages. If they don't play nicely, we should definitely reach out and potentially file a bug!
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.
AFAICT, scoped packages work so long as they are not private. There may be some edge cases but they are in the process of being sanded down.
|
||
Module names should use terms people are more likely to be familiar with. For | ||
example, instead of the ambiguous `platform`, polyfills should be in a module | ||
called `polyfill`. |
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.
I think a big benefit here is that this reorganization could increase the velocity of contributions back to the framework. I've been working with Ember for 4+ years and I still get lost trying to find where to make bug fixes in the source code. An easier to navigate project should help address this
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.
@bcardarella Agreed. And I think so much of Ember finally being replaced by language improvements makes a big difference here as well.
| `Ember.removeListener` | `import { removeListener } from "@ember/object/events"` | | ||
| `Ember.removeObserver` | `import { removeObserver } from "@ember/object/observers"` | | ||
| `Ember.reset` | `import { reset } from "@ember/instrumentation"` | | ||
| `Ember.run` | `import { run } from "@ember/runloop"` | |
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.
Does this still have the remaining functions hanging off of it?
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.
hopefully not :)
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.
I think the answer to that would have to be "no" for this and other packages, because constructing the object would require us to eagerly import everything that was tacked on as a property. However, the codemod (should) already handle this case and import the most-deeply-nested part of the property path correctly (e.g. Ember.run.scheduleOnce
gets replaced with import { scheduleOnce } from "@ember/runloop"
instead of import { run } from "@ember-runloop"
).
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.
That's the answer I was hoping for.
Defining a public API for importing parts of Ember via JavaScript modules helps | ||
us lay the groundwork for solving all of these problems. | ||
|
||
#### Multiple Apps on a Page |
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're looking to reduce the frequency of this type of behavior in browser-side code on the CLI side. Further, because of the way we're using Broccoli, you get even-more pernicious behavior by default (full outer join
) with clobbering at the intersection (overwrite: true
) based upon which is "last" thing we see (level + DAG topsort + package.json ordering).
I'd rather us not present this as a goal/motivation for this RFC.
Short version of goals:
- Shipped-to-browser code should follow the Highlander rule. (
--flat
) - Server-side build related things should (probably) allow for npm-style module resolution.
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.
Can you ELI5 this comment? I'm trying to understand it, but there is too much lingo without context.
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.
I love y'all; I'll try and get some documentation on these issues written down soon. It's a pretty detailed CLI concern and it's likely we attempt to try and change some of the behavior in a way that is potentially in conflict with the message of this section. My only goal with this comment is to remove this section from our stated goals in the RFC because I'm concerned that we may want to walk it back in the future.
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.
@nathanhammond I had some trouble feeling confident that I'd grokked your comment as well. 😁 I think you're saying that we don't want to support the case of building "multiple apps" from a single Ember CLI project. That's something I wholeheartedly agree with.
However, I do think there are a subset of users who want to incrementally migrate to Ember. For better of worse, that often means replacing chunks of server-rendered pages with Ember apps (until enough of the app is in Ember that you can just move things over wholesale.)
For our adoption goals, I think having a story for doing this is important. I am, of course, not strongly opposed to removing this section from the RFC. But the RFC doesn't say that we will support it; it's simply saying that having modules unlocks that potential. My motivation here is to try to enumerate all of the possibilities that are impossibilities under the current system, as a way to persuade the skeptics that there is real value here and not just academic tilting at windmills.
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.
It's not even the "multiple apps" story that is the primary problem. It's the multiple inclusion of modules of different versions of Ember which is a tremendously difficult problem to solve and requires things like symbol mangling. Otherwise all of the different versions of Ember will end up in the same namespace and get clobbered when we overwrite: true
See: https://github.com/ef4/ember-browserify/pull/100/commits. This is, in effect, promising that we would use rollup
(or something similar) to bundle individual versions of Ember (Ember1
... EmberN
) which would be wildly inefficient when we would instead want to only include particular versions of a smaller subset of the modules.
So, this is entirely possible once we land this RFC, but it promises a lot that we're not sure we want to support. My goal would be to only have one version of any browser dependency to constrain bytes sent over the wire: the Highlander rule of "there can be only one!"
I'd also be in favor of a rewrite to simply drop:
its own version without the risk of clobbering another.
Because we in no way guarantee that right now.
|
||
This requires a two-pronged strategy: | ||
|
||
* Tight integration into Ember CLI |
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.
It's possible that we can attempt codemod at build time.
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.
I'm inclined to recommend a 1-time tool, over something during build. Unless your proposing the build write back to its inputs? At which point, we should likely just encouraging the users to run the tool above ^. Maybe a hybrid idea (not quite build time, but part of the ember-cli story), we could potentially integrate this with ember init
, and make this part of a future upgrade step?
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.
The thing is I don't expect we will be able to move 100% of the community all at once. Because of that I'm unsure how best to handle it. If done at build time we don't have to concern our community with it. If it has to be upstreamed into a future release of somebody's addon it'll be hard to move everybody. And thinking about it, yes, it would have to rewrite its inputs and invalidate its output. That's no fun, but more fun than shipping things you didn't need over the wire.
I'm unsure if emeber init
is enough: I use that approximately never.
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.
@nathanhammond ah interesting, this was to support add-ons. So they could continue to support both worlds. I suspect a bit more is needed here, specifically how we warn/deprecate/migrate people off that step in a good way.
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.
Offering a build-time transform is, I think, a brilliant solution to the addon compatibility question I hadn't considered, though it seems obvious now in retrospect.
I don't think it's a good idea for apps, because the process of going from globals to modules is pretty expensive, especially if we do a very thorough job of having the codemod handle the edge cases people discover.
However, the reverse is not true: converting from modules back to globals is trivially easy. If an addon updates to modules, we can provide a Broccoli transform that will rewrite them to globals if the app is on an older version of Ember. I think we can make this step pretty fast, and of course it is eminently cacheable so subsequent builds shouldn't have any performance impact at all.
As for apps, I think we will have an addon that imports everything and constructs the old Ember
global for you. If we detect that you haven't import
ed anything, we can add this an implicit import, with an option to explicitly opt-in if that detection doesn't work for some reason.
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.
I feel like we should move this into the RFC as the plan of record for compatibility. This set of permutations demonstrates that the primary audience we need to make sure we address are in the addon community. This is also beneficial as these people are likely more-experienced Ember developers and will be comfortable making those changes.
Scenario Summary
App
- App written with global Ember? Seems fine, we ship all the things. No clever for you. (Via addon or some other approach, implementation details not particularly relevant to this discussion.)
- App written with modules? We can start to do clever things for you at build time to reduce app size or bundle differently.
Addons
- App written with global Ember and includes addon written with global Ember? No change.
- App written with modules and includes addon written with global Ember? Best attempt at build time codemod the addon to get to keep doing clever things. If it fails the user sets a flag and we stop attempting clever things. This is the least ideal situation.
- App written with global Ember and includes addon written with modules? Guaranteed safe build time codemod back to global Ember.
- App written with modules and includes addon written with modules? No work to be done.
| `Ember.DefaultResolver` | `import GlobalsResolver from "@ember/application/globals-resolver"` | | ||
| `Ember.Enumerable` | `import Enumerable from "@ember/enumerable"` | | ||
| `Ember.Evented` | `import Evented from "@ember/object/evented"` | | ||
| `Ember.HashLocation` | `import HashLocation from "@ember/routing/hash-location"` | |
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.
Bikeshed: not nested under @ember/routing/location/hash
? Would make the app/location/foo
dynamic lookup feel more parallel.
Or maybe we stop allowing that to be dynamic:
import Router from "@ember/routing/router";
import HashLocation from "@ember/routing/hash-location";
export default Router.extend({
location: HashLocation
});
(This is mostly tangential to the RFC. As you were.)
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.
@nathanhammond bring on a new RFC ;P
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.
I was about to ask the same thing. Since they are classes the rules presented here mandate that they should live in their own files, but I'd find more intuitive to have:
"@ember/routing/location" // Ember.Location
"@ember/routing/location/hash" // Ember.HashLocation
"@ember/routing/location/history" // Ember. HistoryLocation
"@ember/routing/location/none" // Ember. NoneLocation
Could the pattern of "If you ship a base class and a few subclasses of it, subclasses should be nested" be a good idea?
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're already following that pattern in this proposal with Ember.Component
... but not with Ember.Object
. 😜
I think this will always come down to taste instead of hard and fast rules which is why I labeled it as a bikeshed.
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.
Perhaps the rule could be A subclass will live in a nested folder within it's parent class, unless is promoted to a top-level "realm" on it's own
.
Or perhaps trying to formalize this is just too much.
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.
I am not opposed to this on principal, but as a constraint during design (which I now realize I failed to enumerate in the RFC) I didn't allow myself to nest more than one level. The reason is purely aesthetics and an intuition that people would find it unnecessarily deep. I am happy to revisit this constraint, but given how rarely people import location objects, it didn't feel worth breaking the rule for.
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.
No strong opinions here; I'm not the target audience who will encounter this (or maybe I am?) but I believe that anybody looking at this portion of Ember will do the right thing either way. Thus my bikeshed label. I'd add the "one level deep" nesting as a design constraint note into the RFC.
Last thought: if published to separate packages the immediate thing that comes to mind is "monorepo?" I don't know how far we wish to take this RFC in terms of organization of the project's codebase itself. Given the |
How are you planning to version the packages? Do the small packages have the same major as the main package? If a breaking change in a small package happens, does it upgrade to a major or a minor? Or they don't have any breaking changes at all? |
How would statically analyzing at build time work for the |
I'd like to attention that Is this because of Would it be better to place both computed and observer both nested in their own package? |
As @martndemus mentioned, I feel that even if In the same logic, I feel that |
I think the main problem with the "old" module shims was discoverability. You couldn't just have a look into |
I'm looking forward to the "published on npm" version of this RFC. Seems like we might finally get default support for importing npm packages (like webpack) 👍. The other benefits would be the nice tooling around this common pattern. IDEs and tools like eslint have nice things built around the npm path and don't work that well with ember's blessed ember aliases. |
If people want to start to experiment with this in their apps (just the imports and package organization) @martndemus released: https://github.com/DockYard/ember-new-modules-shim Keep in mind that as this RFC evolves so will the addon so if you're adverse to churn in your app you may not want to opt in yet. |
This is awesome. The overall intent of the RFC is 100% in the right direction. To me, the 2 major benefits of this process are static analysis and tree shaking, and organisation/discovery of packages (which also affect docs) should have a profound effect on the learning curve for new developers, and sense of separation will be great for seasoned developers also. The need for separating out the framework into separate packages seems like a red herring goal at this stage. We're attempting to make life for EMBER developers easier, not to make ember a multi module framework available on NPM to non-ember projects. For this reason I think shipping as a single npm package for now makes more sense especially with regard to versioning. Breaking things apart in the future then becomes something that's more of an optimisation for other JS users rather than something useful for Ember users (aside from quicker npm install times). Heck, you could even publish the ember-source npm package that just wraps around all the single packages and groups them under one release, freeing ember developers from dependency version hell in their project. It would also allow for ember version releases to lock in sub module versions at release time. Can't wait for this to land. 👍 |
this is all great!! Thanks. Would any of this be applied to templates..? like if there are particular helpers or components that are never used, could we remove them when we build..? |
@webMark I'd assume if the template compilation setup is run before the tree shaking step then certainly helper usage could be filtered and removed. Templates could in theory also be included in this if they were compiled to modules themselves perhaps...? |
One thing to considering adding in one form or another might be In some of our unit tests for ember-data derived classes (models/adapters/etc), we have to create a fake owner for testing. We've looked for good alternate solutions and haven't found anything. Basically we followed the same pattern ember is using internally for testing. See: internal-test-helpers/lib/build-owner.js and ember-data as well: helpers/owner.js. I have also seen a few addons doing this as well. Obviously there is a bigger need here about testing outside the scope of this RFC, but despite these being private mixins and a private test helper, I think one of the two should be exposed. |
Fantastic RFC! :) Should I close #68? |
I agree with @oligriffiths that having a lot of Ember-related packages (potentially each with different version numbers) in I think a meta-package (whatever that would look like) is pretty important for this to be an improvement for our team. |
@joukevandermaas I would suspect that you could have a single |
@joukevandermaas @oligriffiths @BrenoC I suggest reading the Distribution section of the RFC which addresses this issue. |
Sure, I read the distribution section and perhaps I misunderstood but it seems to cover shipping ember as a single source shippable package, that may be later carved out into separate packages. I'm saying once that is done, having a single package definition with all the sub package dependencies listed would be a 👍 thing. Sounds like we're all on the same page on this.
|
Confirm. This is the most likely outcome.
Babel does exactly the same thing, and has released a tool that makes managing this from the publisher side much easier: https://lernajs.io/ |
Look at us, JavaScript developers all agreeeing on something, what a time to be alive. Lol |
This is awesome. 🚀 |
👍 👏 …Can someone update the |
As per discussion related to emberjs/rfcs#176 (comment), LinkComponent should be changed to be a public class.
I realise I might be a little late to the party here, but I think this RFC attempts to tackle too many issues in one go, and that as a result the proposed approach is dangerously complicated. I'd like to outline an alternative. TL;DR
Goals of this RFC at the moment:
This RFC would benefit significantly by reducing its scope from all of the above to single elements described in individual RFCs. All of the goals listed can be tackled independently, without necessarily breaking the public interface of the Ember module. 1. Introduce ES2015 modules without breaking the public APIAs a first step, why not leave the Ember default export as it is, and begin exporting all the included classes, functions etc as named exports? That would allow developers using the ES2015 module syntax to import only what they need, and give the compiler the necessary clues to strip the unimported code from its output. Developers using the default export will obviously not be able to reap this benefit, but it would be a backwards compatible change. As an aid to future migration you could introduce an ESLint rule to warn about importing the default Ember export, prior to completely removing the default export, or defer that decision until a later RFC. The one breaking change would be that you'd need to drop the Ember global. Since your public API remains almost unchanged, the migration for this step would be trivial: import Ember from 'ember';
var { get } = Ember; would become: import { get } from Ember; You wouldn't need to reeducate people, and under the hood you can reorganise the code at your leisure. 2. Don't split the codebase into separate NPM packagesThis task introduces significant complexity to the RFC:
The last point deserves particular attention. This RFC proposes the introduction of no less than 17 NPM packages. It's worth seriously discussing the overhead that maintaining 17 packages will incur: versioning between packages will need to be kept consistent, deploying a change to one package will require updates to potentially many other packages, each package needs to be independently documented, updates need to be propagated to consuming projects - overall the surface area of the project is dramatically increased for no apparent gain. The motivation to split Ember into multiple packages as described in the RFC is to improve consumers' mental model of the Ember codebase, and as a result the organisation of the API docs. Note that this has nothing to do with the performance goals stated at the beginning of the RFC, so should probably be extracted to an RFC of its own. I'd argue that you can group the related code units described in each of the packages in Addendum 2 simply by moving code around in the existing ember package, without the need for any extra NPM packages. Avoiding the extra NPM packages has the following advantages:
Topics which appear to make up the majority of this RFC. 3. Reorganising the codebaseI think the conventions proposed are a good idea, but they should be discussed outside the context of ES2015 modules and multiple NPM packages - both of which are implementation details that need not affect your mental model / code organisation - and probably in a more granular discussion with greater visibility than this RFC. 4. Reorganising the API docsYou can do this completely independently of the rest of this RFC. Considering the importance Ember's documentation has to its community - and the rest of the JavaScript community - I'd say this deserves to be discussed separately, and there have to be easier ways to organise it than introducing a extra packages. I hope this giant post helps :) |
@wmadden sorry for responding shortly, I'm trying to be succinct. I'm more than willing to discuss finer details over on Slack ( 1. Introduce ES2015 modules without breaking the public APIAs addressed in the RFC, we don't break anything, Your solution of 2. Don't split the codebase into separate NPM packagesYou can support multiple npm packages from the same code repository, see babel, lodash, glimmer, and other projects using lerna or a similar tool. Also, you seem to be overlooking the benefit scoped packages can bring, like the clearer separation mentioned in the RFC. 3. Reorganising the codebaseThe goal of eventually reorganising the codebase is so that users don't have to keep mentally mapping packages and namespaces and modules and whatnot. Breaking things into modules lays bare the nothing that Ember is actually many different modules packaged tightly for the benefit of the framework user. We glue things together so you don't have to! I don't understand what you mean by more granular discussion. 4. Reorganising the API docsSee previous point. The existing mismatch is already costly, not sure changing one mismatch with another, more mismatched one would be an improvement for our users. I think the holistic approach taken by @tomdale in elaborating this RFC is very much in the spirit of Ember, and shows the care in addressing the different ways the framework and the ecosystem are affected by the changes, both from the developers and the users perspective. As a member of the Learning team I would like to reiterate that I agree with the teachability points addressed in the RFC, and look forward to what I think will be a brighter future ;) While the RFC itself is already approved and merged, we could certainly use some help delivering it's promises. See you around? ;) |
I think there may be some miscommunication. This is exactly what the RFC is proposing. It does not break any existing API, but introduces new module API on top of the old system. It also says nothing about splitting the codebase into separate NPM packages; in fact, it specifically calls out that packaging is out of scope of this RFC. I think a closer read will show that a lot of the complexity you're concerned about was provided as a motivation for getting the module API defined, but is explicitly out of scope of this RFC. This is purely about defining the API from a consumer's point of view; questions of packaging, distribution, documentation, code organization, etc. are not prescribed here. |
missing |
This replaces the new modules with globals usage when the Ember version in use does not include its own modules. As discussed in a recent ember-cli team meeting.
This replaces the new modules with globals usage when the Ember version in use does not include its own modules. As discussed in a recent ember-cli team meeting.
Add emberjs/rfcs#176 support by default.
Rendered RFC
Author Responses
12/13/2016
11/16/2016
Changelog
1/10/2017
12/13/2016
11/16/2016
getOwner
andsetOwner
to@ember/application
from@ember/object/internal
, per @tchak's suggestionEmber.run
no longer act as namespaces, in response to @nathanhammond's question