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

ENHANCEMENT More advanced detection of whether outputPath is a parent of the project directory #4141

Merged
merged 1 commit into from May 27, 2015

Conversation

catbieber
Copy link
Contributor

Revision of: #4107

Updated tests so that OS-dependent paths (root, and relative paths) should work on *nix and Windows with the same code.

@stefanpenner
Copy link
Contributor

re-running travis (which timed out do to something quirky)

@stefanpenner
Copy link
Contributor

looks like a legit 0.10.x node failure:

Build failed.
Object #<Object> has no method 'parse'
TypeError: Object #<Object> has no method 'parse'
    at Class.module.exports.Task.extend.canDeleteOutputPath (/home/travis/build/ember-cli/ember-cli/lib/models/builder.js:47:27)
    at Class.module.exports.Task.extend.clearOutputPath (/home/travis/build/ember-cli/ember-cli/lib/models/builder.js:66:14)
    at Class.module.exports.Task.extend.processBuildResult (/home/travis/build/ember-cli/ember-cli/lib/models/builder.js:95:17)
    at lib$rsvp$$internal$$tryCatch (/home/travis/build/ember-cli/ember-cli/node_modules/rsvp/dist/rsvp.js:489:16)
    at lib$rsvp$$internal$$invokeCallback (/home/travis/build/ember-cli/ember-cli/node_modules/rsvp/dist/rsvp.js:501:17)
    at lib$rsvp$$internal$$publish (/home/travis/build/ember-cli/ember-cli/node_modules/rsvp/dist/rsvp.js:472:11)
    at Object.lib$rsvp$asap$$flush [as _onImmediate] (/home/travis/build/ember-cli/ember-cli/node_modules/rsvp/dist/rsvp.js:1290:9)
    at processImmediate [as _immediateCallback] (timers.js:354:15)

@catbieber can you address https://github.com/ember-cli/ember-cli/pull/4141/files#r30938561 ?

canDeleteOutputPath: function(outputPath) {
return this.project.root.indexOf(outputPath) === -1;
var rootPathParents = [this.project.root];
var parsedPath = path.parse(this.project.root);
Copy link
Contributor

Choose a reason for hiding this comment

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

require('path').parse isn't something available in node 0.10.x

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, didn't realize that until I saw the failure this morning. Can probably rework using path.dirname() and path.sep instead. Plan to look at that either tonight or tomorrow.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe you can backport the actual parse code into its own module? Might be the safest.

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 can take a look...I'm really just using dir from the parsed path to make sure I get properly uniform and normalized paths (unfortunately path.normalize() wasn't cutting it, and that parents module that looked cool was also normalizing poorly, in a different way from path.normalize()) so path.dirname() might substitute just fine. If I have trouble with that, I'll look at backporting parse.

@catbieber
Copy link
Contributor Author

@stefanpenner It looks like tests are passing -- let me know if there are more improvements I should make to this patch.

@stefanpenner
Copy link
Contributor

@catbieber thanks! This looks good :)

stefanpenner added a commit that referenced this pull request May 27, 2015
ENHANCEMENT More advanced detection of whether outputPath is a parent of the project directory
@stefanpenner stefanpenner merged commit 7d998be into ember-cli:master May 27, 2015
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

2 participants