onedesign / generator-one-base

A foundation for One Design Company projects on the web.
Other
1 stars 1 forks source link

Don't hardcode creator values #139

Open cmalven opened 6 years ago

cmalven commented 6 years ago

We're currently hard-coding the following values:

We need a documented, simple way for creators outside of ODC to use this generator with custom values for these.

brianjhanson commented 6 years ago

I think we should go with npm config values if they're set. Looks we could potentially grab a bunch of stuff from there:

Looks like we could also create a sub-module that would get run on npm init example & docs

Also makes me wonder if we should just start a child process that runs npm init rather than asking the questions ourselves, seems like we could lean on npm for a lot of that stuff.

cmalven commented 6 years ago

Would be nice if it was easy. Here are the issues I see in general with relying on npm init for this info:

  1. we can't easily get values out of package.json generated by npm init which we need for other portions of generator output, such as README.
  2. we can't easily put information into npm init, so it's difficult to use npm init for most project details and continue to rely on prompts for the resuable stuff.
  3. npm init requires an entry point which we don't have a need for.
  4. we usually want our final package.json to include custom scripts, which we can't configure using npm init, so we'd need to do something gross to add these if package.json was created using npm init

Looks like we could rely on the init-package-json to mostly get around these, at the expense of understanding/introducing an additional tool and the complexity it brings. For me, the value we'd get from doing this is pretty questionable. Yeoman prompts are easy to understand and configure, using Yeoman to generate the package.json from a template is easy to understand/configure, and (IMO at least) a global .yo-rc-global.json does a great job of remembering my generator defaults – while still making them easy to override – without expecting people to configure an additional file.

If we could find a way to leverage npm to reduce the code + complexity in the generator I'm in favor, but if not I think the tools we're already using do the job well.

brianjhanson commented 6 years ago

yeah, I mostly meant we should lean on the npm init default values for those that have them set up.

const npmConf = require('npm-conf');
const conf = npmConf();
const authorName = conf.get('init-author-name');

if (!authorName) {
  prompts.push({ // author name prompt });
}

Using something like the npm conf package.

I don't think it's a terribly common thing to do, but I think it's better practice than creating our own way for defaults to work. Ideally we would support both npm init defaults and .yo-rc-global.json / .yo-rc.json files. Even better if it asks if you want to save your answers as defaults for next time around.

I'm not saying we definitely should user npm init only that I feel like we're starting to duplicate the functionality it already provides. Just trying to be conscious of rewriting something that we're not doing substantially better.

If we run npm init and create the package.json we can easily read any values from the package.json file. We can also easily add to / edit a generated package.json file.

const pkg = require('package.json');
const extend = require('deep-extend');

// Get values and put them in the readme
fs.copyTpl('README.md', 'README.md', { name: pkg.name });

// We could even only prompt for package.json stuff it the file doesn't already exist
if (!fs.existsSync('package.json') {
   prompts.concat(packageJsonPromts);
}

// To add new properties to the package.json
const withScripts = extend(pkg, {
   scripts: {
       start: "gulp stuff"
       dev: "another task"
   }
});

I agree that the templating version is easier to understand, but I also think it can be more complex if we have a lot of logic in those templates that decide what does and doesn't get output in which situations.

cmalven commented 6 years ago

yeah, I mostly meant we should lean on the npm init default values for those that have them set up.

Absolutely agree that we should use the npm conf values as defaults if they exist.

I'm not saying we definitely should user npm init only that I feel like we're starting to duplicate the functionality it already provides.

We agree here, too, but for me the solution of using npm init probably adds more complexity than it removes, not in terms of lines of code but in understanding how the generator arrives at its final results. The solution you posted is as nice as it could be, but would still require writing to an existing file in the filesystem (package.json), which seems like a yeoman anti-pattern, and I feel like a "guiding principle" of the generator should be that the template files we include in the generator are as close as possible to the final result, like I mentioned in #140. Otherwise I think it becomes difficult to wrap your head around the final resulting files that will be output.