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

Libraries included by default should not be in users Brocfile. #536

Merged
merged 1 commit into from
May 1, 2014

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Apr 30, 2014

No description provided.

@@ -6,6 +6,7 @@
`Router.reopen({location: 'none'});` from test helpers [#516].
* [ENHANCEMENT] Update loom-generators-ember-appkit to `^1.1.1`.
* [BUGFIX] Whitelist `ic-ajax` exports to prevent import validation warnings. [#533](https://github.com/stefanpenner/ember-cli/pull/533)
* [BUGFIX] Files added by ember-cli should not needed to be specified in `Brocfile.js`. [#534](https://github.com/stefanpenner/ember-cli/pull/534)
Copy link
Contributor

Choose a reason for hiding this comment

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

is this really a bugfix?

Copy link
Member Author

Choose a reason for hiding this comment

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

I personally consider bad user experience a bug, yes. But I don't care too much, if you'd like it to be [ENHANCEMENT] that is totally fine with me.

Copy link
Contributor

Choose a reason for hiding this comment

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

[ENHANCEMENT] 👍 because it worked although it was sloppy

@jakecraige
Copy link
Member

Is there a way to remove these after the fact in the user's Brocfile? Say I didn't want to include ember-data, how would I do that? If we can, I think it should be documented.

Will this prevent running the beta/canary builds since it's required to be called ember.js?

@twokul
Copy link
Contributor

twokul commented Apr 30, 2014

@jakecraige yes, scumbag me :) you're gonna have to remove libraries you don't want from the legacyFilesToAppend before calling app.toTree(). Also, you will need to delete those libraries from your bower.json.

@stefanpenner
Copy link
Contributor

how do they then remove "ember-data" if they dont want it?

@MajorBreakfast
Copy link
Contributor

@stefanpenner Good point. I think we'll only get this clean once we have our addon system.

@rwjblue
Copy link
Member Author

rwjblue commented Apr 30, 2014

@stefanpenner it is quite simple:

var EmberApp = require('ember-cli/lib/broccoli/ember-app');
var remove = require('lodash').remove;

var app = new EmberApp({
  name: require('./package.json').name,
});

remove(app.legacyFilesToAppend, function(elem) {
  return elem === 'ember-data.js';
});

module.exports = app.toTree();

@twokul
Copy link
Contributor

twokul commented Apr 30, 2014

@rjackson @stefanpenner that's what I meant

@rwjblue
Copy link
Member Author

rwjblue commented Apr 30, 2014

This comes down to fundamentals. We ship with Ember Data by default, and I would assume it is used in the majority of cases (which is why it is the default). The things that are most common should be very simple (and require little to no effort). Things that are outside the normal/default stack are OK to be slightly more difficult (but definitely still need to be possible).

This solution still seems VERY simple, and allows even more cleanup in the end users Brocfile. Seems like a win/win to me.

@stefanpenner
Copy link
Contributor

@rjackson another approach is to parse the bower.json and see if ember-data is installed and use that to conditionally add/remove stuff. This pattern is what we use for preprocessors and stuff, and should scale a bit better.

@rwjblue
Copy link
Member Author

rwjblue commented Apr 30, 2014

@stefanpenner - That seems fine (and I'll update if you want), but my point is more that we have to make it possible to diverge from our default path, but it doesn't need to be the optimized case.

@stefanpenner
Copy link
Contributor

I would prefer we consult the bower.json for now, seams like something that will grow with us a bit more.

@rwjblue
Copy link
Member Author

rwjblue commented Apr 30, 2014

Just updated to leave Ember data in Brocfile.js for now, that seemed to be the only controversial one (even though I personally think we either ship it by default or not). IMHO, this is still a win (only one boilerplate line instead of 10-15).

@rwjblue
Copy link
Member Author

rwjblue commented Apr 30, 2014

@stefanpenner - Is this an OK compromise for now?

if (this.env !== 'production') {
ignoredModules.push(
'qunit',
'ember-qunit'
'ember-qunit',
'ic-ajax'
Copy link
Contributor

Choose a reason for hiding this comment

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

ic-ajax should be in production. At least I'd like to use it because $.ajax returns non-spec-compliant Promises.

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved.

@twokul
Copy link
Contributor

twokul commented May 1, 2014

@rjackson rebase?

@twokul
Copy link
Contributor

twokul commented May 1, 2014

LGTM

@rwjblue
Copy link
Member Author

rwjblue commented May 1, 2014

@stefanpenner - Is this acceptable now (I moved Ember Data back into the users Brocfile.js)?

@stefanpenner
Copy link
Contributor

it's ok, this addon thing gives me a massive headache. I just want afew days to improve it...

stefanpenner added a commit that referenced this pull request May 1, 2014
Libraries included by default should not be in users Brocfile.
@stefanpenner stefanpenner merged commit 52955a6 into ember-cli:master May 1, 2014
@rwjblue rwjblue deleted the do-not-litter-in-brocfile branch July 3, 2014 02:00
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.

7 participants