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.

Sign up for GitHub

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

Jump to bottom

Test assets moved to test-vendor.js and test-vendor.css. #1288

Merged
merged 4 commits into from
Jul 6, 2014
Merged

Test assets moved to test-vendor.js and test-vendor.css. #1288

merged 4 commits into from
Jul 6, 2014

Conversation

jakecraige
Copy link
Member

Add type option to import to specify which that it's a test asset.

This PR moves the qunit assets into a test-vendor.css and test-vendor.js. This allows qunit and qunit-notifications to be moved into their own addons. It also allows people to import test only assets via their Brocfile.

app.import('vendor/mytestasset/index.js', {type: 'test'});

I'm not entirely happy with the API and would love some feedback on what y'all think.

rwjblue
rwjblue reviewed Jul 6, 2014
View reviewed changes
lib/broccoli/ember-app.js
@@ -490,7 +486,7 @@ EmberApp.prototype.import = function(asset, options) {
assetPath = asset;
}

if (assetPath === null) {
if (assetPath == null) {
Copy link
Member

Choose a reason for hiding this comment

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

Why not just if (assetPath)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that'd work

On Sun, Jul 6, 2014 at 10:17 AM, Robert Jackson notifications@github.com
wrote:

In lib/broccoli/ember-app.js:

@@ -490,7 +486,7 @@ EmberApp.prototype.import = function(asset, options) {
assetPath = asset;
}

  • if (assetPath === null) {
  • if (assetPath == null) {

Why not just if (assetPath)?

—
Reply to this email directly or view it on GitHub
https://github.com/stefanpenner/ember-cli/pull/1288/files#r14575652.

Thanks,

Jake Craige
Poetic Systems
675 Bering Dr. Suite 725
Houston, TX 77057
(281) 531-4060 - Office

@rwjblue
Copy link
Member

rwjblue commented Jul 6, 2014

Travis is failing because with this refactor we do not need moveFile anymore. You can remove both the variable at the top here, and the package from package.json. That should allow Travis to be green.

@jakecraige
Copy link
Member Author

@rjackson Removed the reference in ember-app.js. Have to keep it in package.json because of this

@rwjblue
Copy link
Member

rwjblue commented Jul 6, 2014

Gotcha, good catch.

@rwjblue
Copy link
Member

rwjblue commented Jul 6, 2014

Will definitely need a changelog entry. This also paves the way for using alternative testing frameworks (as you could just app.import them now.

rwjblue
rwjblue reviewed Jul 6, 2014
View reviewed changes
lib/broccoli/ember-app.js
@@ -137,6 +136,14 @@ function EmberApp(options) {
this.otherAssetTrees = [];
this._importTrees = [];

this.legacyTestFilesToAppend = [
Copy link
Member

Choose a reason for hiding this comment

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

Could this be made to use the vendorFiles object from above? Then in populateLegacy files we would just call app.import and things would 'just work'?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the lack of clarity, I mean couldn't we define these arrays as empty arrays, and use the populate legacy files function along with app.import to populate it.

@rwjblue
Copy link
Member

rwjblue commented Jul 6, 2014

Will need to be rebased since #1159 / #1293 have been merged.

@jakecraige
Copy link
Member Author

@rjackson looks good now

rwjblue
rwjblue reviewed Jul 6, 2014
View reviewed changes
CHANGELOG.md
@@ -7,7 +7,9 @@
* [ENHANCEMENT] When using `app.import` to import non-js/css assets, you can now specify the destination of the asset. [#1159](https://github.com/stefanpenner/ember-cli/pull/1159)
* [BUGFIX] Fix issue with `ember build` failing if the public/ folder was deleted. [#1270](https://github.com/stefanpenner/ember-cli/issues/1270)
* [BREAKING ENHANCEMENT] CoffeeScript support is now brought in by `ember-cli-coffeescript`. To use CoffeeScript with future versions run `npm install --save-dev ember-cli-coffeescript` (and `broccoli-coffee` is no longer needed as a direct dependency). [#1289](https://github.com/stefanpenner/ember-cli/pull/1289)
<<<<<<< HEAD
Copy link
Member

Choose a reason for hiding this comment

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

Merge artifacts.

rwjblue added a commit that referenced this pull request Jul 6, 2014
@rwjblue
Merge pull request #1288 from jakecraige/test-vendor-assets
4382775 
Test assets moved to test-vendor.js and test-vendor.css.
@rwjblue rwjblue merged commit 4382775 into ember-cli:master Jul 6, 2014
@rwjblue
Copy link
Member

rwjblue commented Jul 6, 2014

Thanks!

@jakecraige jakecraige deleted the test-vendor-assets branch July 6, 2014 23:06
@rwjblue rwjblue mentioned this pull request Jul 7, 2014
Closed
@bcardarella
Copy link
Contributor

@jakecraige out of curiosity why test-vendor.js instead of test.js ?

@bcardarella
Copy link
Contributor

@jakecraige a little more background. I would like to use these trees for the testing add-ons I am writing. The name test-vendor.js currently implies this is only used for vendored libraries.

@jakecraige
Copy link
Member Author

@bcardarella i just wanted them to be under a single name and vendor seemed good since there was already a vendor.js and the only difference with this was that they were test specific.

I like just test.js though that sounds like it's the test files themselves which may be confusing

@bcardarella
Copy link
Contributor

@jakecraige maybe two trees then? A tests.js which would be all of the test files then a test-support.js which would be everything else?

@rwjblue
Copy link
Member

rwjblue commented Jul 9, 2014

Currently we cannot build our tests to a different file than our app. This is mostly due to how the current and future concatenator's work. They require knowledge of all imports and exports, then transpile and concat them together.

Today we output AMD, but we will be outputting a loader less bundled format fairly soon (hopefully).

@bcardarella
Copy link
Contributor

Does anyone care if this gets renamed test.js then? I am having a hard time the current name implying it is limited to vendor assets only.

@rwjblue
Copy link
Member

rwjblue commented Jul 9, 2014

I personally do not care too much what the filename is. Not sure 'tests.js' is good though. Perhaps 'test-support.js' (as you suggested earlier)...

@bcardarella
Copy link
Contributor

👍

🚲shed PR pending...

@bcardarella bcardarella mentioned this pull request Jul 9, 2014
Merged
@jakecraige
Copy link
Member Author

jakecraige commented Jul 9, 2014 •
edited by bcardarella
Loading

I have no attachment to test-vendor so if the I there's agree, that's fine.

I agree with @rjackson though that just tests.js is a bit misleading

On Tuesday, July 8, 2014, Brian Cardarella notifications@github.com wrote:

[image: 👍]

[image: 🚲]shed PR pending...

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

@rwjblue rwjblue mentioned this pull request Jul 16, 2014
Closed
@jakecraige jakecraige mentioned this pull request Jul 28, 2014
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jakecraige @rwjblue @bcardarella