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

how should we handle config.usePodsByDefault when pods are enabled by default? #2652

Closed
EndangeredMassa opened this issue Dec 3, 2014 · 23 comments

Comments

@EndangeredMassa
Copy link
Contributor

When making pods the default project structure in ember-cli, how should we handle the config option usePodsByDefault? We have a couple of options.

  1. switch to a different config option like disablePodsByDefault; (I don't know what to call the opposite of pods.)
  2. just change the app blueprint to set usePodsByDefault: true
  3. deprecate the flag and assume pods going forward

I think that Option 2 is fine, but would love to hear what other people think about it.


/cc @trabus @stefanpenner

@stefanpenner
Copy link
Contributor

i think we should deprecate the flag and assume pods going forward.

@shunchu
Copy link

shunchu commented Dec 3, 2014

It would be great if those who have already been using Pods for a while share some tips and/or insights on how to break features down to pods. Sometimes features don't fit nicely into a single "pod" per se. Any insights would be appreciated. Better yet, it'd be great to have some of those "best practices" and insights documented somewhere to help enforce the idea and on-board new devs with this awesome tool.

@stefanpenner
Copy link
Contributor

@shunchu ya this is a great point. "What is a pod" "how to think about a pod" are great points to attempt to make clear.

@trabus
Copy link
Contributor

trabus commented Dec 3, 2014

We should probably agree on a name for the "classic" type structure when pods become the default. I think it still makes sense to allow generating blueprints in the original structure, and having a name we could use as a flag would make the transition easier for some people.

@jakecraige
Copy link
Member

I'm thinking it would be nice to keep the option around, especially when upgrading apps that don't currently use the pod structure. A Boolean called usePods would do the trick. No need for the byDefault part imo

@leandrocp
Copy link
Contributor

Agree with @jakecraige to keep the option around. It´s easier to discover if a project use pods or not.

My feedback: I work with various ember-cli projects, and my day-to-day workflow is to open up config/environment.js to see how a project is configured.

@stefanpenner
Copy link
Contributor

better question, why is the podConfig in the app/config ? seems like a build tool concern.

cc @rwjblue

@trabus
Copy link
Contributor

trabus commented Jan 31, 2015

@stefanpenner This was my fault. I misunderstood something when I was adding it and actually wasn't aware of .ember-cli at the time.

@stefanpenner
Copy link
Contributor

@stefanpenner This was my fault. I misunderstood something when I was adding it and actually wasn't aware of .ember-cli

got time to deprecate and move? ;)

@stefanpenner
Copy link
Contributor

I'm trying to re-start the pod fleshout effort this weekend.

example project -> https://github.com/stefanpenner/ember-jobs

@trabus
Copy link
Contributor

trabus commented Jan 31, 2015

Awesome. Yeah, I'll take a crack at it, I'm at a resort with my family on a Girl Scouts outing, but they're all asleep so I'll see how far I can get tonight. ;)

@trabus
Copy link
Contributor

trabus commented Jan 31, 2015

Going with usePods to replace usePodsByDefault. I had to do some tweaks to get the environment settings to be available to the blueprint, but I have it working. It should probably be easier to get .ember-cli properties inside ember-cli, but passing in the settings through the command and task is working for now.

@trabus
Copy link
Contributor

trabus commented Jan 31, 2015

Just a note, this issue isn't resolved with moving the config property to .ember-cli. We still need to derive some sort of antonym for --pod that people can use if they want to generate (or destroy) modules in the type based format.

This actually goes back to the initial addition of pod support, where it was proposed as a --structure=pod flag. We could refactor to use that instead, using shorthands for the generate and destroy commands to set the flag. Generate available options would look like:

// assuming 'type' as the antonym for 'pod'
availableOptions: [
    { name: 'structure', type: String, default: 'type', aliases: ['s',  {'pod' : 'pod' }, {'type' : 'type'}}
]

@trabus
Copy link
Contributor

trabus commented Feb 1, 2015

Because of existing options for the route blueprint, -type won't work as a shorthand. I was having some difficulty trying to find a suitable name other than -type, but ultimately I think the only one that would make any sense would be -default.

I feel that using the shorthands and keeping the current --pod flag intact for a period of time would allow the easiest transition. Using the --structure option would also open the door for generating in other structures that may come about (maybe engines would have a specific structure for instance).

@EndangeredMassa
Copy link
Contributor Author

I like the --structure=pod/type idea. I can foresee some different structures that people may want to use, which could address #2654. Do we need shorthand options? Whichever you prefer to do most of the time should be stored in your .ember-cli. So, you will only be typing it out long-form when you want to do the opposite of your configuration.

Supporting --pod with a deprecation warning would be nice.

@trabus
Copy link
Contributor

trabus commented Feb 2, 2015

I absolutely want to have shorthand options for structure that map to the defined structure. Using -pod and -basic is much easier than --structure=pod/basic, or even -s pod/basic. I don't think we immediately need to get rid of --pod, but definitely should deprecate it eventually. Internally, blueprints should have some way to define (and expand) structures that map to how fileMapTokens are processed. Maybe something like this:

Blueprint.prototype.getStructure = function(options) {
  var availableStructures = this._structures(options);
  // if the structure passed in options isn't supported fall back to default
  if (availableStructures[options.structure].isSupported(this)) {
    return availableStructures[options.structure];
  }
  return availableStructures['default'];
}

Blueprint.prototype._structures = function(options){
  var standardStructures = {
    pod : {
      // test to determine if this blueprint can support this structure
      isSupported: function(blueprint) {
        return blueprint.hasPath;
      },
      // values used in creating tokens
      name: function(options) {
        return options.blueprintName;
      },
      path: function(options) {
        return path.join(options.podPath, options.dasherizedModuleName);
      },
      test: function(options) {
        return options.blueprintName;
      }
    },
    default : {

      isSupported: function() {
        return true;
      },
      name: function(options) {
        return options.dasherizedModuleName;
      },
      path: function(options) {
        var blueprintName = options.blueprintName;
        if(blueprintName.match(/-test/)) {
          blueprintName = options.blueprintName.slice(0, options.blueprintName.indexOf('-test'));
        }
        return inflector.pluralize(blueprintName);
      },
      test: function(options) {
        return options.dasherizedModuleName + '-test';
      }
    }
  }
  var customStructures = this.structures(options) || options.structures || {};
  return merge(standardStructures, customStructures);
}

Blueprint.prototype._fileMapTokens = function(options) {
  var currentStructure = this.getStructure(options);
  var standardTokens = {
    __name__: function(options) {
      return currentStructure.name(options);
    },
    __path__: function(options) {
      return currentStructure.path(options);
    },
    __test__: function(options) {
      return currentStructure.test(options);
    }
  };

  var customTokens = this.fileMapTokens(options) || options.fileMapTokens || {};
  return merge(standardTokens, customTokens);
};

Edited to remove confusion over default as the standard structure, changed it to basic

@trabus
Copy link
Contributor

trabus commented Feb 2, 2015

@jgwhite @stefanpenner @rwjblue Thoughts ^^?

@EndangeredMassa
Copy link
Contributor Author

I don't think that we should call the non-pod structure default when the goal is to make pod the default.

@trabus
Copy link
Contributor

trabus commented Feb 2, 2015

But even when pods are default, there will still be several blueprints that generate in the standard type structure because they don't support pod structure. Maybe -standard or -basic instead of -default, but regardless, the type based structure will still be around, and we can't use -type for the reasons I mentioned above (router blueprint).

@lukesargeant
Copy link

We've been making the move to pods. I like bits of both. Neither quite capture the two most important concepts which i believe are components and resources.

Here's my spanner for the works on how i'd structure it.

app

  • components
    • component-name
      • template.hbs
      • component.js
      • styles.css
  • resources
    • resource-name
      • route.js
      • model.js
      • adapter.js
      • serializer.js
  • styles

Moving to a new default structure would at least mean the usePodsByDefault config wouldn't need to be touched.

@trabus
Copy link
Contributor

trabus commented Feb 8, 2015

@lukesargeant I'm working on an RFC for a Structure model now. The way I'm designing it, you would be able to define your own structure that would be picked up be the resolver without any additional setup. You'd define the structure rules, and drop it into your project. I'm also thinking you'd be able to define your structure as the defaultStructure in your .ember-cli file, achieving the same result as usePodsByDefault but work for any structure, not just pods.

I should note, I still need to determine how much work it would be to make the resolver be configurable (to just pick up new rules and prioritize them). It may not be possible, or something people would be open to.

@chadhietala
Copy link
Member

@trabus I think to make the resolver configurable devs would have to provide a custom pattern function and then push that cat that pattern on to the front of the default patterns.

Devs could write something like this

export default function(parsedName) {
    // return your custom lookup here
};

Internally we would blueprint a subclass of the resolver that did something like.

import customPattern from <%structure_resolver>
var Resolver = require('ember/resolver')['default'] // might be able to do `import Resolver from 'ember/resolver'`;
export default Resolver.extend({
  customPattern: customPattern,
  moduleNameLookupPatterns: Ember.computed(function(){
    var defaults = this._super();
    return [this.customPattern].concat(defaults);
  })
})

@abuiles abuiles added this to the v1.0.0 milestone Apr 12, 2015
@abuiles
Copy link
Member

abuiles commented Apr 12, 2015

@trabus is working on this and as part of it I'm sure he would include an answer for this, I'm closing this issue in favor of his work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants