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

ES6 modules #68

Closed
wants to merge 1 commit into from
Closed

ES6 modules #68

wants to merge 1 commit into from

Conversation

zeppelin
Copy link


```js
import Component from 'ember-component';
import { isBlank } from 'ember-utils';
Copy link
Member

Choose a reason for hiding this comment

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

export property buckets, aren't really super tree-shaking friendly. I don't mind, but its worth considering, ember-utils/is-blank for those who want to benefit from the simple tree shaking.

It may be nice to keep the buckets, so users who don't care have an easy experience.

Copy link
Author

Choose a reason for hiding this comment

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

Is there no way to do tree-shaking for named exports or just harder than for defaults?

Copy link
Member

Choose a reason for hiding this comment

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

significantly more difficult.

It requires much more machinery/complexity

Copy link
Author

Choose a reason for hiding this comment

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

Could you point to an sample implementation? I'm happy to dig in, since I feel introducing more than one way to access Ember stuff brings only confusion to newcomers especially.

@chadhietala
Copy link
Contributor

While I agree with this, it would probably be better if ember was a single addon that provided the different built packages. From a maintainability point this is much better what developers add to their project is not much different. Instead of import Component from "ember-component"; you would simply say import Component from "ember/component";. This is also way more inline with how ember is currently setup. Essentially everything in https://github.com/emberjs/ember.js/tree/master/packages gets rollup into it's own file.

@rwjblue
Copy link
Member

rwjblue commented Jun 23, 2015

@chadhietala - We definitely discussed that at length, but we prefer this modules listing as it allows us to split things up properly in the future without changing our public module API. Right now, NPM doesn't really allow us to split them, but we definitely plan to do so as they add better support...

Here, both `Ember.Component` and `Ember.isBlank` could be `import`ed:

```js
import Component from 'ember-component';
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be mentioned that Ember's repo will change to reflect this new structure.

Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

Choose a reason for hiding this comment

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

Basically, the shims repo update will allow 1.x apps to start using the new module structure but Ember 2.x will provide these exports and will also maintain this internal structure.

@tomdale
Copy link
Member

tomdale commented Jun 23, 2015

Instead of import Component from "ember-component"; you would simply say import Component from "ember/component";

While I prefer the aesthetics of ember/component to ember-component, I think the best option is to use the dash form since it gives us the opportunity to distribute the pieces of Ember as separate npm modules in the future.

As you noted above, we plan to re-arrange Ember's internal structure to match the proposed module structure in this RFC. We have two options for distributing that: as one big package that we treeshake for unused code, or as a series of smaller packages with an ember metapackage that includes all of them.

Obviously, the move from Ember CLI apps consuming globals to modules is a fairly significant change for people to make. If we use / as a delimiter, we've foreclosed on distributing that module as an npm package. With dashes, should we have a need to distribute smaller packages on npm, we can do so without having everyone do another mass refactor.

@stefanpenner
Copy link
Member

What about using npm's scoped packages.

ember-component -> @ember/component

Note, as for 5 minutes ago I am squatting the scope ember on npm.

features:

  • looks official
  • we control packages that are available on the ember scope
  • NPM has plans to add more organization features (discovery, user management)

Cons:

  • won't work with npm 1.4 (but what does)
  • we had thought of using @ for namespaces, but NPM has already taken that meaning.

@stefanpenner
Copy link
Member

Stef Penner [2:37 PM]

I'm concerned about @ember/component vs a single module cross module deps

Stef Penner [2:38 PM]
because NPM + peer deps isn’t much fun

More research and exploration needed.

@jayphelps
Copy link

I've discussed this in the past with numerous core team members and am still unconvinced it should be cut into npm-style packages as the default way to consume.

i.e. ember/component 👍 / ember-component 👎

Sure, if you want to package them separately as that via npm, go for it, but the default ember build that ships with ember-cli should IMO allow you to access everything under the ember/ namespace.

Having experimented with broccoli-babel tree shaking myself, I'm not sure how splitting them into separate npm packages helps automated tree shaking in this way. It's arguably more complex to shake if everything was just on ember, like import { Component } from 'ember', but thankfully that's not being proposed here (yet) :trollface:.

Ultimately, I don't think the developer experience should suffer simply because it's a bit easier to shake the tree at the npm module boundry vs. inner-module boundaries.

Side note bikeshed that won't make it, but have to say it: I disagree with ember-cli's convention of hyphenating the filename and thus the fully qualified path of modules. It's using ES2015 modules unlike most module systems and makes things less clear for no apparent benefit. This tradition seems to be carried over to ember core as well. I think there's much benefit to ember/Component vs. ember/component style; keeping function/class names case intact as-is. It makes it clear whether the default import is a function vs. class, helps when you're importing index/bucket modules, and reducing learning curve for anyone who uses modules in any other language that has them. I've never been a full time rubyist, but I'm guessing this convention comes from there possibly? If so, I'm ever so slightly less against it...but still don't think it's a great convention to proliferate.

Edit: @rwjblue and I have been discussing this on Slack. Nothing really to add, but I understand the original argument better now though.

Most importantly:
shaking

@MajorBreakfast
Copy link

  • It's probably clearest to use the ember/component methodology. ember/component could then just export * from 'ember-component' if it gets extracted into its own package.
  • I want to add that everything discussed here should work with JavaScript's own upcoming loader. There's not much gain in building the system around node's cjs require because that's not what one day will land in JavaScript engines like v8. No drastic adjustments needed for this but it should be kept in mind. (loader, polyfill). It should be possible to support both. And I'm only mentioning it because the upcoming loader uses promises for dynamic loading (not using import syntax, that's static loading).

@jayphelps
Copy link

I know +1's suck, but I forgot to mention all of what @MajorBreakfast just said I agree with as well.

@bcardarella
Copy link
Contributor

I like @stefanpenner's suggestion of the npm namespace, but in the event that doesn't work out I don't see why we can't have the best of both worlds. If Ember itself is going to be distributed as an ember-cli addon, we can override the targeted es6 module prefix to transpile to. So for any number of node packages:

  • ember-component
  • ember-utils
  • ember-data

we can set the modulePrefix of each addon to just 'ember/<type>' and the transpiled code should be accessible via:

  • ember/component
  • ember/utils
  • ember/data

Then the import statements should be what people are accustomed to.

@stefanpenner
Copy link
Member

@bcardarella one downside, is @ from npm scopes, may interfere with the @ in the proposed (somewhat implemented) namespaces. Maybe not, but its worth thinking about

@mixonic
Copy link
Sponsor Member

mixonic commented Jul 6, 2015

@zeppelin Can we also add an export for Ember.HTMLBars.SafeString at ember-htmlbars/safe-string? Ember.HTMLBars.SafeString also needs to be added to Ember, though we will not deprecate Ember.Handlebars.SafeString until later in the 2.x cycle (it is very cheap to keep around).

@zeppelin
Copy link
Author

zeppelin commented Jul 6, 2015

@mixonic Absolutely, will do once I got home!

On Mon, Jul 6, 2015 at 6:10 PM Matthew Beale notifications@github.com
wrote:

@zeppelin https://github.com/zeppelin Can we also add an export for
Ember.HTMLBars.SafeString at ember-htmlbars/safe-string?
Ember.HTMLBars.SafeString also needs to be added to Ember, though we will
not deprecate Ember.Handlebars.SafeString until later in the 2.x cycle
(it is very cheap to keep around).


Reply to this email directly or view it on GitHub
#68 (comment).

@stefanpenner
Copy link
Member

I believe the general tone is now, @ember/component the @ember would be the NPM scope (that I am already squatting)

We clearly need to fixup some parts of ember-cli that don't quite work with npm scopes.

I would love to hear other concerns with this approach, I am sure we are missing something.

@cibernox
Copy link
Contributor

cibernox commented Jul 7, 2015

I am not very aware of downsides of each approach, but import Component from 'ember/component' feels more natural to me because it matches the global Ember.Component

@Gaurav0
Copy link
Contributor

Gaurav0 commented Jul 7, 2015

I'm in favor of whichever syntax would not have to require us to change it again in the future.

@stefanpenner
Copy link
Member

@Gaurav0 that would be the point of attempting to enumerate potential issues while in RFC form.

Do you see any potential issues we may be missing?

@Gaurav0
Copy link
Contributor

Gaurav0 commented Jul 8, 2015

@stefanpenner I don't feel I understand npm well enough to know why a dash is better than a slash, much less know about any missing potential issues. Just wanted to weigh in that I would prefer not having to change again over something that feels more "natural".

@tomdale
Copy link
Member

tomdale commented Jul 8, 2015

I'm in favor of @stefanpenner's @ember/component proposal, since it preserves the aesthetics without sacrificing the ability to break apart into separate packages later. It also means that it's impossible for someone, maliciously or not, to squat on future ember-* package names we may end up needing.

@chadhietala
Copy link
Contributor

I like the scope proposal. I would like modulePrefix to go into the 🔥 as it becomes extremely difficult to map an import back to a package/addon.

@locks
Copy link
Contributor

locks commented Jul 9, 2015

@tomdale @stefanpenner how does the import work, import Component from '@ember/component'? I like the npm scope approach as well.

@fsmanuel
Copy link

fsmanuel commented Jul 9, 2015

I'm also in favor of npm scopes and like import Component from '@ember/component'. Can someone link to existing tree-shaking approaches i would like to dig into that.

@stefanpenner
Copy link
Member

Can someone link to existing tree-shaking approaches i would like to dig into that.

TL;DR

  1. build a graph of all dependencies
  2. figure out what is reachable
  3. drop what isn't reachable

cc @chadhietala i tried to link to your recent graph work, but it appears the most recent may not yet be pushed?

@chadhietala
Copy link
Contributor

@stefanpenner Yes that is correct. Still fixing up some tests but it is here. The "prune unreachable" piece is here

@fsmanuel
Copy link

@stefanpenner @chadhietala thanks

@chadhietala
Copy link
Contributor

After thinking about this more I don't think taking the approach implemented here is correct because it is not a correct representation of the real graph but rather an explosion of complexity without any real advantage to the end user. I propose that Ember actually builds the packages described in app-shims and starts to be less complected.

In the packager work I've introduced the concept of a dist folder in addons that allow you to supply built files. The dist folder must contain a dep-graph.json that describes the contents of the built files. This file is automatically generated by broccoli-babel-transpiler. If your addon were to supply a dist on publish it would look something like this.

dist/
  array.js
  component.js
  dep-graph.json
  ...

dep-graph.json

{
  "ember/array": {
    "exports": ["Array"],
    "imports": []
  },
  "ember/component": {
    "exports": ["Component"],
    "imports": []
  }
  ...
}

Downstream in the linker we consume this information to determine what is actually in the graph and thus shaking the broccoli trees to only output a tree that is an actual representation of the real graph.

Introducing more shims simply makes dealing with real graph resolution much more difficult because now you need exceptions into the tree shaking process that point at a single shadowing file.

@stefanpenner
Copy link
Member

Thanks for this RFC, I am closing in-favor of: #176 as may of the ideas are incorporated, and you also appear on board #176 (comment)

Thanks for helping flesh this out!

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.

None yet