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

Deprecate funnel and pickfiles #3681

Merged
merged 1 commit into from Mar 28, 2015
Merged

Deprecate funnel and pickfiles #3681

merged 1 commit into from Mar 28, 2015

Conversation

stefanpenner
Copy link
Contributor

No description provided.

@stefanpenner stefanpenner force-pushed the deprecate-funnel-and-pickfiles branch from 2b2c1af to 33d85ab Compare March 27, 2015 21:32
@stefanpenner
Copy link
Contributor Author

@rwjblue r?

rwjblue
rwjblue reviewed Mar 27, 2015
View reviewed changes
lib/models/addon.js
@@ -112,14 +116,17 @@ Addon.prototype._requireBuildPackages = function() {

this.transpileModules = this.transpileModules || require('broccoli-es6modules');
Copy link
Member

Choose a reason for hiding this comment

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

Needs to be deprecated also

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch.

@stefanpenner stefanpenner force-pushed the deprecate-funnel-and-pickfiles branch from 33d85ab to a2b2a1c Compare March 27, 2015 22:08
@stefanpenner
Copy link
Contributor Author

@rwjblue updated

@stefanpenner stefanpenner force-pushed the deprecate-funnel-and-pickfiles branch 8 times, most recently from 7c61da9 to 5c1a1cb Compare March 27, 2015 23:16
rwjblue
rwjblue reviewed Mar 27, 2015
View reviewed changes
lib/models/addon.js
@@ -103,24 +107,43 @@ Addon.prototype.hintingEnabled = function() {
@private
@method _requireBuildPackages
*/

Addon.prototype._requireBuildPackages = function() {
Copy link
Member

Choose a reason for hiding this comment

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

Since this now does nothing should we deprecate it also?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i need it actually to get good error messages.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I don't see that in this pull request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

been running this on several apps at work, and working hard to make nice deprecation output (and catching other cases i didn't think of)

I think i got them all now

@stefanpenner stefanpenner force-pushed the deprecate-funnel-and-pickfiles branch 2 times, most recently from f1fe1af to 9ec187f Compare March 27, 2015 23:39
@stefanpenner
deprecate both this.Funnel and this.pickFiles
4c687df 
this wasn’t really intended for public API, users should depend on their own packages.
@stefanpenner stefanpenner force-pushed the deprecate-funnel-and-pickfiles branch from 9ec187f to 4c687df Compare March 27, 2015 23:56
stefanpenner added a commit that referenced this pull request Mar 28, 2015
@stefanpenner
Merge pull request #3681 from ember-cli/deprecate-funnel-and-pickfiles
2c5b558 
Deprecate funnel and pickfiles
@stefanpenner stefanpenner merged commit 2c5b558 into master Mar 28, 2015
@stefanpenner stefanpenner deleted the deprecate-funnel-and-pickfiles branch March 28, 2015 00:21
@samselikoff
Copy link

I changed this in my addon but consumers are having trouble. I tried adding broccoli-merge-trees and broccoli-funnel to devDependencies and dependencies, but if they used

npm install samselikoff/ember-cli-mirage

to install master, they get the error

Cannot find module 'broccoli-merge-trees'

Any thoughts on how to fix? And, is it okay to use this.Funnel and this.mergeTrees for now?

@samselikoff
Copy link

actually, adding to dependencies did fix. Is this the suggested path? If so I can add some clarification in the docs

@stefanpenner
Copy link
Contributor Author

Yup, this is how npm works. DevDependencies don't get installed when a consumer installs your module as a dependency of another.

I am hesitant to add documentation that already exists elsewhere. Maybe this is an exception, and we treat it more as a reminder?

@samselikoff
Copy link

What about a line here for dependencies that says

"dependencies": {
  // add npm packages your addon depends on here

? It could be me but I'm still not clear on what goes there vs. devDependencies. Basically if my addon uses an npm package in index.js, it needs to be in dependencies?

This absolutely could have nothing to do with ember-cli and is just my lack of understanding of npm

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.

None yet
4 participants
@stefanpenner @samselikoff @rwjblue @joostdevries