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

Change bowerDirectory assignment order in case .bowerrc doesn't contain directory #1902

Merged
merged 1 commit into from
Sep 6, 2014

Conversation

jayphelps
Copy link
Member

This fixes the rare case that someone has a .bowerrc file but it doesn't actually have a directory prop defined. Before, the code would continue with this.bowerDirectory === undefined

@jayphelps
Copy link
Member Author

I have no idea how you could write a sane unit test for this. If you do, lead the way!

@@ -61,11 +61,11 @@ function EmberApp(options) {
this.tests = options.hasOwnProperty('tests') ? options.tests : !isProduction;
this.hinting = options.hasOwnProperty('hinting') ? options.hinting : !isProduction;

this.bowerDirectory = 'bower_components';

if (fs.existsSync(path.join(this.project.root, '.bowerrc'))) {
this.bowerDirectory = JSON.parse(fs.readFileSync(path.join(this.project.root, '.bowerrc'))).directory;
Copy link
Member

Choose a reason for hiding this comment

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

This will still call .directory against the return value of fs.readFileSync wouldn't it?

I think we need:

// this goes outside the constructor
function readJSONFile(path) {
  var result = {};

  try {
    if (fs.existsSync(path)) {
      var contents = fs.readFileSync(path, { encoding: 'utf8' });

      result = JSON.parse(contents);
    }
  } catch(e) {
    // ignore error
  }

  return result;
};

Copy link
Member Author

Choose a reason for hiding this comment

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

Absolutely would. I wasn't covering malformed .bowerrc cases, just if it didn't have 'directory` defined.

e.g.

{
  "analytics": false
}

I can certainly do so, but I think we shouldn't silently swallow any exception. c?

Copy link
Member

Choose a reason for hiding this comment

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

Then we just use that func and default:

this.bowerDirectory = readJSONFile(path.join(this.project.root, '.bowerrc')).directory || 'bower_components';

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you are correct, we still want an error if the .bowerrc is malformed (and my mechanism would just swallow the whole thing).

rwjblue added a commit that referenced this pull request Sep 6, 2014
Change bowerDirectory assignment order in case .bowerrc doesn't contain `directory`
@rwjblue rwjblue merged commit 39c892e into ember-cli:master Sep 6, 2014
@rwjblue
Copy link
Member

rwjblue commented Sep 6, 2014

👍 - Thanks!

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