gulpjs / liftoff

Launch your command line tool with ease.
MIT License
843 stars 52 forks source link

support locating multiple configuration files #8

Closed tkellen closed 8 years ago

tkellen commented 10 years ago

configuration like this:

new Liftoff({
  configFiles: {
    '.app': {
      extensions: ['rc','.yml'],
      search: {
        cwd: {
          path: './',
          findUp: true
        },
        myHomeKey: '~',
        etc: '/etc'
      }
    },
    'appfile': {
      extensions: function () {
        return Object.keys(require.extensions)
      },
      searchPaths: {
        baseDir: {
          path: './',
          findUp: true
        },
        home: '~',
        alternate: '/path/to/alternate',
        multiple: ['/path/to/whatever', '/path/to/something', '/path/to/something/else']
      }
    }
  }
});

yields environment like this:

env.configFiles = {
  '.app': {
    cwd: '/var/www/project/code/.apprc',
    myHomeKey: '/Users/user/.apprc',
    etc: null
  },
  'appfile': {
    baseDir: '/var/www/project/code/appfile.js',
    home: null,
    alternate: null,
    multiple: ['/path/to/whatever/appfile.js', '/path/to/something/appfile.js']
  }
};

~ expands to process.env.HOME || process.env.HOMEPATH || process.env.USERPROFILE searchPath entries which specify an array of paths should NOT perform findUp on each entry.

jonschlinkert commented 10 years ago

@tkellen what's the logic behind having both of these?

alternate: '/path/to/alternate',
pickTheFirst: ['/path/to/whatever','/path/to/something','/path/to/something/else']

I might be missing the point of alternate, but wouldn't each subsequent path in the pickTheFirst array be an alternate to the previous? when alternate be used?

shama commented 10 years ago

userhome might be a good module for determining the user's home cross platform. :)

tkellen commented 10 years ago

boom. done! thanks @shama :)

tkellen commented 10 years ago

@jonschlinkert alternate isn't a special key name, you can call it whatever you want. same with pickTheFirst except that it will search each path in order and stop on the first one it finds the requested config in.

jonschlinkert commented 10 years ago

@shama nice, I can use that on a few projects.

alternate isn't a special key name

ahhhh. each of the props on searchPaths is (now obviously) arbitrary. got it, thx

tkellen commented 10 years ago

What happens to the searchPaths for appfile if they pass in { appfile: '/path/to/appfile.js' }? Are they ignored because the user explicitly provided them?

tkellen commented 10 years ago

Per @cowboy's feedback, there needs to be a way to configure if a given searchPath entry should attempt to findup from the specified paths.

jonschlinkert commented 10 years ago

@tkellen, per @cowboy's suggestion, sounds super-useful but should that be a separate issue since it goes beyond what's needed here?

what happens to the searchPaths for appfile if they pass in { appfile: '/path/to/appfile.js' }?

thinking out loud, you could add searchPath and searchPaths. If an implementor wants to explicitly provide a path, they could do so on searchPath:

'appfile': {
      searchPath: '/path/to/appfile.js'
}

If searchPath is defined, searchPaths will be ignored. Or maybe call it defaultPath or something that is more easily distinguished.

tkellen commented 10 years ago

I've just expanded the configuration format to cover the edge case @cowboy pointed out. By default, no searchPath will be "found up" from unless explicitly configured to do so.

I agree with @jonschlinkert that if an implementor provides a path for a given configFile entry, any supplemental search paths defined should be ignored.

jonschlinkert commented 10 years ago

Hey @tkellen, you mentioned that you did some work on the configuration format, would you want to push that up to a branch? I'm not sure what this will entail yet, but I could try to do a pr to that branch if I can some time to do it.

tkellen commented 10 years ago

Hey @jonschlinkert, sorry for the slow reply here. I'm out in Santa Monica on a consulting gig and I haven't had time to take this further, yet. I am returning home tomorrow evening--I will do my best to get an initial implementation pushed next week.

jonschlinkert commented 10 years ago

No worries at all! no rush, I really appreciate it!

sttk commented 8 years ago

@tkellen I tried this issue and finished implementing it on my repository.

sttk/js-liftoff#support_multiple_config_files

But I changed a bit of your specification:

sttk commented 8 years ago

For the example in the top comment, the program outputs as follows:

(/test/var/www/project/code/tester.js)
var Liftoff = require('liftoff');
var util = require('util');

var require = {
  extensions: {
    '.js': null,
    '.json': null,
  },
};

var Tester = new Liftoff({
  name: 'tester',
  configFiles: {
    '.app': {
      extensions: ['rc', '.yml'],
      searchPath: {
        cwd: {
          path: './',
          findUp: true,
        },
        myHomeKey: '~',
        etc: '/etc',
      },
    },
    'appfile': {
      extensions: function() {
        return Object.keys(require.extensions);
      },
      searchPaths: {
        baseDir: {
          path: './',
          findUp: true,
        },
        home: '~',
        alternate: '/test/path/to/alternate',
        multiple: ['/test/path/to/whatever', '/test/path/to/something', '/test/path/to/something/else'],
      },
    }
  },
});

Tester.launch({}, function(env) {
  console.log(util.inspect(env, { depth: null }));
});
$ find /test/path
/test/path
/test/path/to
/test/path/to/something
/test/path/to/something/appfile.js
/test/path/to/something/else
/test/path/to/whatever
/test/path/to/whatever/appfile.js

$ find /test/var/www/project/code -maxdepth 2
/test/var/www/project/code
/test/var/www/project/code/.apprc
/test/var/www/project/code/appfile.js
/test/var/www/project/code/node_modules
/test/var/www/project/code/node_modules/liftoff
/test/var/www/project/code/tester.js
/test/var/www/project/code/testerfile.js
/test/var/www/project/code/workdir

$ ls ~/.apprc
/Users/sttk/.apprc

$ cd /test/var/www/project/code/workdir
$ node ../tester
{ cwd: '/test/var/www/project/code',
  require: [],
  configNameSearch: [ 'testerfile.js', 'testerfile.json' ],
  configPath: '/test/var/www/project/code/testerfile.js',
  configBase: '/test/var/www/project/code',
  modulePath: undefined,
  modulePackage: {},
  configFiles: 
   { '.app': 
      { cwd: '/test/var/www/project/code/.apprc',
        myHomeKey: '/Users/sttk/.apprc',
        etc: null },
     appfile: 
      { baseDir: [ '/test/var/www/project/code/appfile.js' ],
        home: [],
        alternate: [],
        multiple: 
         [ '/test/path/to/whatever/appfile.js',
           '/test/path/to/something/appfile.js' ] } } }
sttk commented 8 years ago

@tkellen Thanks for giving me push access, but I want you to check whether my modification meets your specification before commit this to the repository.

And now, I'm considering that searched paths should be returned not like 'searched/file/path.json' but like { extension: '.json', path:'searched/file/path.json' } for applying require functions corresponding to their extensions after. How do you think about it?

tusbar commented 8 years ago

@sttk could you open a PR here so we can review the changes properly? :)

sttk commented 8 years ago

@tusbar I've opened this PR. Please review.

phated commented 8 years ago

Fully fleshed out idea from https://github.com/js-cli/js-liftoff/pull/67#discussion_r59125527

Possible new spec:

new Liftoff({
  // extensions are already supported by liftoff
  extensions: {
    '.js': null,
    '.yml': 'yml-loader-module'
  },
  configFiles: {
    '.app': {
      // value can be an object with `path`, `findUp` and `extensions` properties
      cwd: {
        path: './',
        findUp: true,
        // explicit extensions that override top-level extensions
        // extension overrides would be handled exactly the same as top-level extensions
        // everything else defaults to top-level extensions
        extensions: {
          'rc': 'some-special-rc-loader'
        }
      },
      // value can be a string that expands to `{ path: value }`
      // if extensions aren't passed using an object, top-level extensions are used
      myHomeKey: '~',
      etc: '/etc'
    },
    'appfile': {
      baseDir: {
        path: './',
        findUp: true
      },
      home: '~',
      alternate: '/path/to/alternate',
      // value can also be an array of strings or objects (strings get expanded)
      multiple: ['/path/to/whatever', '/path/to/something', '/path/to/something/else']
    }
  }
});

yields

env.configFiles = {
  '.app': {
    cwd: '/var/www/project/code/.apprc',
    myHomeKey: '/Users/user/.app.yml',
    etc: null
  },
  'appfile': {
    baseDir: '/var/www/project/code/appfile.js',
    home: null,
    alternate: null,
    multiple: ['/path/to/whatever/appfile.js', '/path/to/something/appfile.js']
  }
};

/cc @tkellen @sttk

sttk commented 8 years ago

I understand new spec as follows (and have some questions) :

  1. searchPaths/searchPath properties are abolished.
    • Question: If multiple paths are hit with extensions, is the result an array of paths, or only a first path?
    • Question: If the result path can be a string or an array, do users have to check it before using it?
  2. Each configFiles properties (e.g. .app) can be an object.
    • Question: Does it support a string and an array? (it is supported now.)
  3. Each properties of configFiles properties (e.g. cwd and myHomeKey) can be an object, a string, a string array, or an object array. If an object (including an element of an array), path property is mandatory in it.
  4. configFiles property (e.g. .app) is used as a part of path.
    • Question: Is it allowed to include '.' and '~' characters? (it is allowed now)
phated commented 8 years ago

Thinking deeper about your questions, I think every result should be a singular string. That would mean dropping the array syntax (and relying on the keyed object for fallbacks) or stopping traversal after the first match; which do you think is better? I'm leaning towards dropping the array syntax.

Extensions: match the behavior that liftoff currently uses. If there is a gulpfile.js and a gulpfile.babel.js in a project, which is used?

I don't think configFiles should accept an array or string and should only accept an object.

The properties of configFiles are supposed to be the file's basename, as it is currently written. If we wanted to do away with that and use generic keys (like rc, etc), we would have to add a name property to the objects and I don't really like that. I'm on the fence about using the key as the file's basename but it is probably for the best. If it stays that way, all characters are part of the filename (and are appended with the extensions), nothing in the key is expanded (e.g. ~ is not expanded to home) and only valid path characters should be allowed as keys.

Hope that helps, but it might open up other questions.

sttk commented 8 years ago

@phated Thank you for answering my questions.

That would mean dropping the array syntax ... or stopping traversal after the first match; which do you think is better?

If every result is a singular string, I also think that dropping the array syntax is better because it makes less confusing for users and I can't think the cases in which the syntax is used.

... If there is a gulpfile.js and a gulpfile.babel.js in a project, which is used?

Liftoff preferentially uses an earlier extension in specified order. (Though it isn't guaranteed.)

I don't think configFiles should accept an array or string and should only accept an object.

I got it. I'll do so.

... nothing in the key is expanded (e.g. ~ is not expanded to home) and only valid path characters should be allowed as keys.

I got it. I'll implement not to expanded '~' and '.' but to use the key just as user specify.

phated commented 8 years ago

Support for this was just published as 2.3.0