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

[Deprecation] Introduce new build file #4286

Merged
merged 6 commits into from
Jun 25, 2015

Conversation

chadhietala
Copy link
Member

To be able to pass around the project instance we need to parameterize the build file. Since Brocfile means something to broccoli we need to create a new build file.

This will allow EmberApp to call the Brocfile repeatedly and share the project across rebuilds.
@stefanpenner stefanpenner mentioned this pull request Jun 12, 2015
16 tasks
@stefanpenner
Copy link
Contributor

I think we can actually do this via deprecation. We should be able to inspect the imported module.

@stefanpenner
Copy link
Contributor

The goal of this is to parameterize Project.

I believe @rwjblue has some other ideas.

@rwjblue
Copy link
Member

rwjblue commented Jun 12, 2015

Brocfile.js has specific meaning in Broccoli-land, we should not completely change that meaning. What we want is our own thing that replaces Brocfile.js and accepts a project instance. We should just make it a new file (preferably in config/ somewhere).

@stefanpenner
Copy link
Contributor

We could call it ember.js or 'build.js' or something, Potentially at the root or in the config dir.

@rwjblue
Copy link
Member

rwjblue commented Jun 12, 2015

Using ember.js would then reference two different files, one published by Ember itself and one used by ember-cli. We should not continue to overload the same terms/names with different meanings.

/* global require, module */
var EmberApp = require('ember-cli/lib/broccoli/ember-app');
Copy link
Member

Choose a reason for hiding this comment

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

This is using ember-app and should be ember-addon.

@chadhietala
Copy link
Member Author

@rwjblue Moving it to config gets complicated as you can change the config directory name by passing configPath to EmberApp. So you have a chicken and egg problem when you attempt to look up the build file prior to actually running the build. This could be solved more generically with using a glob pattern e.g. /**/build.js but I have a feeling that build.js is way too generic for us to cut over safely.

I would suggest that we have a new build file in the root of the project called ember-cli-build.js or even <name-of-app>-build.js. The first option I feel has the least chance of overlap.

@stefanpenner
Copy link
Contributor

@chadhietala maybe the global route skips this problem for now? It does make me cringe though.

Ultimately this improvement will need to happen. We just need to nail what it means

@chadhietala chadhietala changed the title [Breaking] Brocfile should return a function that recieves the project [Decrecation] Introduce new build file Jun 12, 2015
@chadhietala
Copy link
Member Author

@stefanpenner I think it's doable we just need a different name or path that we can guarantee will be there and does not have the likelihood to cause collisions.

@rwjblue
Copy link
Member

rwjblue commented Jun 12, 2015

ember-cli-build.js

This is the best option proposed so far. I personally dislike having tons of files in a project root, but if we need an unambiguous thing then this is it...

@stefanpenner
Copy link
Contributor

@chadhietala it is actually ambiguous in config?

@rwjblue I am unsure which is better, root of dir or in config/. Balancing clutter and visibility is a tricky thing. Ultimately it likely will just be up to us to make a choice.

@rwjblue
Copy link
Member

rwjblue commented Jun 12, 2015

Ultimately it likely will just be up to us to make a choice.

Agreed. I'd be perfectly happy removing the configPath option (or making it only applicable to config/environment.js which AFAIK is what it applies to).

@chadhietala
Copy link
Member Author

We use configPath because we have another config folder that cannot change and used for other machinery. So I would be 👎 on that.

@chadhietala chadhietala changed the title [Decrecation] Introduce new build file [Deprecation] Introduce new build file Jun 12, 2015
@chadhietala
Copy link
Member Author

@rwjblue and @stefanpenner restarting travis, locally my tests are green. We just need to deal the one of hardest problems in programming... naming things. Also I'm wondering if it's necessary to add a test to test the deprecation, logic is pretty straight forward.

@chadhietala
Copy link
Member Author

@rwjblue and @stefanpenner, do you guys have any more thoughts on this?

```
var EmberApp = require('ember-cli/lib/broccoli/ember-app');

module.exports = function(project) {
Copy link
Contributor

Choose a reason for hiding this comment

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

how about project -> { project: project }

which ends up being:

module.exports = function(defaults) {
  var app = new EmberApp(merge(defaults, {
    ....
  }));

  module.exports = app.toTree();
}

Copy link
Contributor

Choose a reason for hiding this comment

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

we could also make EmberApp do the merge....

@chadhietala chadhietala force-pushed the pass-project branch 3 times, most recently from ad38c45 to 35d7625 Compare June 24, 2015 21:24
@stefanpenner
Copy link
Contributor

tests are unhappy

Instead of passing N arguments to the EmberApp constructur, we should pass a hash so we do not paint our selves into a corner. Internally this is merged from right to left with the developer's options. In other words the developer can completly override the defaults.
@chadhietala
Copy link
Member Author

Everything looks good if we're ok with the drop in code coverage.

@stefanpenner
Copy link
Contributor

@rwjblue r?

var buildFilePath = findUp(file);

if (buildFilePath === null) {
throw new Error(file + ' not found');
Copy link
Member

Choose a reason for hiding this comment

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

Throwing here means that we do not allow fallback to Brocfile.js with deprecation, can we have a test that has just a Brocfile.js and confirms that this still works?

rwjblue added a commit that referenced this pull request Jun 25, 2015
[Deprecation] Introduce new build file
@rwjblue rwjblue merged commit d5c840d into ember-cli:master Jun 25, 2015
@stefanpenner
Copy link
Contributor

Yay

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

6 participants