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

Add pod blueprint generators #1994

Merged
merged 1 commit into from
Sep 19, 2014
Merged

Conversation

trabus
Copy link
Contributor

@trabus trabus commented Sep 14, 2014

Introduces the option for using a pod structure when generating a blueprint by passing the --pod option, as described in #1619 (This PR is a cleaned up version of #1879)

ember g resource taco --pod
/*
 would generate-
 app/pods/taco/route.js
 app/pods/taco/template.hbs
 tests/unit/routes/taco-test.js
*/

The pod option uses the original blueprint, but transforms the tokens used for the mapFile method. Previously this was limited to __name__, which was used throughout the default blueprint files folder. There are two additional default tokens required for blueprints that support pod structure: __path__ and __test__. These three tokens allow quite a bit of flexibility, including support for nested pod generation:

ember g resource taco/cheese --pod
/* 
 would generate-
 app/pods/taco/cheese/route.js
 app/pods/taco/cheese/template.hbs
 tests/unit/routes/taco/cheese-test.js
*/

The tokens are used to generate a fileMap based on the structure defined. If --structure is not passed, the default structure will be used. The __test__ token is necessary because the name token changes per structure:

// pod
fileMap: {
  __name__: 'controller',
  __path__: 'pods/taco',
  __test__: 'taco'
}
// default
fileMap: {
  __name__: 'taco',
  __path__: 'controllers',
  __test__: 'taco'
}

The fileMap (which is defined on the locals), is used for the mapFile method, which writes out all the paths for the files to be generated when the blueprint is installed. MapFile simply loops through the defined tokens and replaces the token instances in files with the defined values.

Blueprint.prototype.mapFile = function(file, locals) {
  var pattern, i;
  var fileMap = locals.fileMap || {__name__: locals.dasherizedModuleName};
  file = Blueprint.renamedFiles[file] || file;
  for(i in fileMap){
    pattern = new RegExp(i, 'g');
    file = file.replace(pattern, fileMap[i]);
  }
  return file;
};

This PR also introduces the ability to define custom tokens to be used in the files to be installed. This is achieved through a new blueprint hook called fileMapTokens. The hook will be processed along with the locals, and add or override any tokens the author wishes. To add a token, the author must add the hook method to their blueprint index.js, defining each token, and a function that resolves the token value. See route and component blueprints for an example.

module.exports = {
  fileMapTokens: function() {
    return {
      __token__: function(options) {
          // this could be a series of checks and returns
          // as long as it returns a value
          return 'value';
      }
    };
  },

@rwjblue
Copy link
Member

rwjblue commented Sep 17, 2014

@jgwhite - Can you review and +1/-1? You know this area of the code much better than I....

@trabus
Copy link
Contributor Author

trabus commented Sep 17, 2014

Rebased with updated index.js for blueprints (no longer using Blueprint.extend()).

var locals = { dasherizedModuleName: stringUtils.dasherize(moduleName) };
this.pod = options.pod;
this.podPath = generatePodPath();
this.hasPath = hasPathToken(this.files());
this.project = options.project;

Copy link
Contributor

Choose a reason for hiding this comment

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

Cosmetic: align =

@jgwhite
Copy link
Contributor

jgwhite commented Sep 17, 2014

❤️ This is an awesome piece of work. ❤️

I’ve made a couple of cosmetic comments more to aid myself absorbing all the changes than anything else.

Some thoughts:

podModulePrefix

I believe this should be defined in config/environment.js. Then we can simply say require('config/environment.js').podModulePrefix in blueprint.js and config.podModulePrefix in app.js.

@rwjblue @stefanpennerconfig/environment.js as canonical home for podModulePrefix: +1/-1?

test token

Could the __test__ token add -test? Then we can drop it from the blueprint filenames.

Warnings

Now that test blueprints have been separated, the “this blueprint does not support pods” warning is a bit noisy. Can we add a flag to silence these warnings? This would allow the parent blueprint to silence its child’s warnings.

@rwjblue
Copy link
Member

rwjblue commented Sep 17, 2014

@jgwhite - I submitted #2024 to ensure that podModulePrefix is pulled from config/environment.js.

@jgwhite
Copy link
Contributor

jgwhite commented Sep 17, 2014

@rwjblue awesome, hadn’t spotted that. @trabus could you rebase and work that in?

@rwjblue
Copy link
Member

rwjblue commented Sep 17, 2014

@jgwhite - You didn't miss it, I just did it per your request above (seemed like a good idea).

@trabus
Copy link
Contributor Author

trabus commented Sep 17, 2014

@jgwhite - I will get right on these, shouldn't be too tough to get rebased and make the changes.

@trabus
Copy link
Contributor Author

trabus commented Sep 17, 2014

'You specified the pod flag, but this blueprint does not support pod structure. ... Warnings are now behind the --verbose flag.

The podModulePrefix is now pulled in from config, and the podPath is built in _locals() now, which made the generatePodPath() method unnecessary (and has now been removed). It's much cleaner now.

I went through the blueprint files that used the __test__ token and removed -test, as it is now added to the token value (concatenating the dasherizedModuleName and '-test' now).

@trabus
Copy link
Contributor Author

trabus commented Sep 17, 2014

Should I go through and rename the test files that currently use __name__-test, or just leave them be for now? They currently work as is, since the blueprints using them don't support pods so the __name__ token uses the dasherizedModuleName.

@jgwhite
Copy link
Contributor

jgwhite commented Sep 17, 2014

Leave the non-pod test blueprints as-is. I’ll give this another review later tonight.

@trabus
Copy link
Contributor Author

trabus commented Sep 17, 2014

I'm currently watching #2027 for changes that I'll need to apply to my pods-generate-test.js and pods-destroy-test.js. I've already applied the rimraf changes, and will apply any others that I see come up.

@jgwhite
Copy link
Contributor

jgwhite commented Sep 17, 2014

👍

@trabus trabus force-pushed the pod-blueprints branch 2 times, most recently from ef85db3 to 732a6d3 Compare September 18, 2014 16:19
@rwjblue
Copy link
Member

rwjblue commented Sep 19, 2014

@trabus - Can you squash commits?

@jgwhite - Where are we at with this?

@trabus
Copy link
Contributor Author

trabus commented Sep 19, 2014

@rwjblue - Done!

@jgwhite
Copy link
Contributor

jgwhite commented Sep 19, 2014

One more review today. If there's nothing outstanding then we'll be good to go.

The fileMapToken order should match fileMapTokenValues order
if overriding. The fileMapTokenValues order should match fileMapTokens
order. Default is expected, pods is necessary if pod structure
is supported for blueprint.
Copy link
Contributor

Choose a reason for hiding this comment

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

This explanation is a little confusing. Where is fileMapTokenValues specified? Could you give this inline doc a little rewrite?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, good catch, this documentation is from an earlier version of the method.

@jgwhite
Copy link
Contributor

jgwhite commented Sep 19, 2014

Aside from that one comment on the inline documentation this is a home run. Tweak that, add a CHANGELOG entry, and we’re good to merge. Thanks for putting so much time and consideration into this.

@jgwhite
Copy link
Contributor

jgwhite commented Sep 19, 2014

Note: the changelog entry should go under a ### Master heading up top.

@jgwhite
Copy link
Contributor

jgwhite commented Sep 19, 2014

LGTM

jgwhite added a commit that referenced this pull request Sep 19, 2014
@jgwhite jgwhite merged commit 426caeb into ember-cli:master Sep 19, 2014
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.

3 participants