Skip to content
This repository has been archived by the owner on Nov 11, 2017. It is now read-only.

Allow Configuration #39

Merged
merged 1 commit into from
Jan 9, 2015
Merged

Conversation

seanpdoyle
Copy link
Contributor

Changes driven by attempting to integrate with emberjs/data

  • configure an output build name and namespace
  • skip building template compiler
  • skip building the runtime

@twokul
Copy link
Member

twokul commented Jan 9, 2015

I'll review later tonight.

@seanpdoyle seanpdoyle force-pushed the fixes-for-ember-data branch from 07cb21c to 72910e3 Compare January 9, 2015 14:44
@seanpdoyle
Copy link
Contributor Author

@twokul there was a typo that was causing a failure.

This should be passing now.

@seanpdoyle seanpdoyle force-pushed the fixes-for-ember-data branch from 72910e3 to 19e759f Compare January 9, 2015 14:46
@@ -29,6 +29,9 @@ function EmberBuild(options) {

if (options && options.packages) {
this._packages = options.packages;
this._name = options.name || 'ember';
Copy link
Member

Choose a reason for hiding this comment

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

this._name, this._namespace and this._skipRuntime should be initialized as null in init method so object shape stays the same.

@seanpdoyle seanpdoyle force-pushed the fixes-for-ember-data branch from 19e759f to 01c7d09 Compare January 9, 2015 15:05
this._name = null;
this._namespace = null;
this._skipRuntime = null;
this._skipTemplates = null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@twokul @rwjblue are skipTemplates and skipRuntime a good idea to pass in as options?

Should they live in build-config.js?

Copy link
Member

Choose a reason for hiding this comment

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

@seanpdoyle actually, good point. It seems like we don't need to dynamically include/exclude runtime or template compiler (happens once per build). So I think it's fine for it to be a environment flag.

@seanpdoyle seanpdoyle force-pushed the fixes-for-ember-data branch 3 times, most recently from 65a64c7 to 40b24e7 Compare January 9, 2015 15:24
@seanpdoyle
Copy link
Contributor Author

@twokul moved to CLI arguments and ready for another look

@seanpdoyle
Copy link
Contributor Author

@twokul actually, moving the config to ENV variables is problematic.

We're moving config from Ember Data's Brocfile.js to the package.json.

npm start becomes SKIP_TEMPLATES=true SKIP_RUNTIME=true NAME=ember-data NAMESPACE=DS ember serve. This is ok, except for the fact that npm build doesn't get the ENV variables, and neither does npm test.

ember serve doesn't pass them along to the pre-serve ember build

So we can either leave the config in Brocfile.js, or duplicate the variables across package.json scripts

"build": "SKIP_TEMPLATES=true SKIP_RUNTIME=true NAME=ember-data NAMESPACE=DS ember build",
"pretest": "SKIP_TEMPLATES=true SKIP_RUNTIME=true NAME=ember-data NAMESPACE=DS ember build",
"start": "SKIP_TEMPLATES=true SKIP_RUNTIME=true NAME=ember-data NAMESPACE=DS ember serve"

@twokul
Copy link
Member

twokul commented Jan 9, 2015

@seanpdoyle I thought you could set environment variables in .travis.yml, like here? Am I mistaken?

@seanpdoyle
Copy link
Contributor Author

@twokul won't that only come into effect in CI?

@twokul
Copy link
Member

twokul commented Jan 9, 2015

@seanpdoyle yeah, we should probably keep it in Brocfile.js then.

@seanpdoyle seanpdoyle force-pushed the fixes-for-ember-data branch from 40b24e7 to 01c7d09 Compare January 9, 2015 15:43
Changes driven by attempting to integrate with [emberjs/data][1]

* configure an output build name and namespace
* skip building template compiler
* skip building the runtime

[1]: https://github.com/emberjs/data
@seanpdoyle seanpdoyle force-pushed the fixes-for-ember-data branch from 01c7d09 to 5dcd0de Compare January 9, 2015 15:44
twokul added a commit that referenced this pull request Jan 9, 2015
@twokul twokul merged commit 5a75b90 into emberjs:master Jan 9, 2015
@twokul
Copy link
Member

twokul commented Jan 9, 2015

@seanpdoyle thanks man!

@seanpdoyle
Copy link
Contributor Author

@twokul this is still of a work in progress. Expect more config based PRs until I can figure out how to get ember data building properly.

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

Successfully merging this pull request may close these issues.

2 participants