General Discussion about Project Direction

RyanZim commented 7 years ago

For those of you that don't know, @ben-eb added me as a collaborator here. I would like to move the project forward towards the v3 major release.

My primary goals for v3 are:

[ ] Support @michael-ciniawsky's common config https://github.com/michael-ciniawsky/postcss-load-config https://github.com/postcss/postcss-cli/issues/45
[ ] Fix bugs in --watch and use the new message.type.dependency https://github.com/postcss/postcss-cli/issues/44
[ ] Get proper tests, written in JS instead of Make

@watilde started with a refactor of the entire codebase in https://github.com/postcss/postcss-cli/pull/46. @michael-ciniawsky has created a proof-of-concept cli supporting the common config; rewriting from scratch. https://github.com/michael-ciniawsky/postcssrc-cli.

postcssrc-cli's code seems cleaner, it already supports the common config, and I like the interface better. It still needs a few bugs fixed, needs to add support for the --use option, and needs tests; but I think those could be implemented fairly easily.

I don't know if this is a wise idea or not, but I'm thinking perhaps we should just move postcssrc-cli into this repo (or somehow) and make an improved version of postcssrc-cli the "official" cli. Would like to know everyone's thoughts on this.

Attn: @ai @pirxpilot

michael-ciniawsky commented 7 years ago

Streams

glob-stream || vinyl-fs [ + pump]

lib/postcss.js

const { Transform } = require('stream')

const postcss = require('postcss')
const postcssrc = require('postcss-load-config')

module.exports = function (ctx) {
  const stream = new Transfrom()

  stream._transform = function (file, enc, cb) {
    postcssrc(ctx).then((config) => {
      postcss(config.plugins)
       .process(file.toString(), config.options)
       .then((result) => cb(null, result))
    })
  })

  return stream
}

index.js

const postcss = require('./lib/postcss')
...
const input = argv.input ? fs.createReadStream(argv.input) : process.stdin
const output = argv.output ? fs.createWriteStream(argv.output) : process.stdout

input
  .pipe(postcss(ctx))
  .pipe(output)
...

`postcss.config.js` (Common Config)

--config|-c should scaffold a postcss.config.js file if missing, alternatively plugins/options set via '--use|-ushould create apostcss.config.js` file if none present.
Needs a 90% finished trivial update to expose the config.file being able to watch the config for changes aswell, when --watch|-w is enabled.
--use|-u or --ctx|--config|-c should support setting the context from the command line to override options set via ctx.? in postcss.config.js

const defaults = { dir: path.dirname(argv.input) || argv.dir, ext: path.extname(argv.input) }

const ctx = argv.ctx ? Object.assign(argv.ctx, defaults) : defaults

postcssrc(ctx).then((config) => {
  // Watch postcss.config.js
  watcher.add(config.file)

  postcss(config.plugins)
   .process(fs.readFileSync(file), config.options)
   .then((result) => {
    // Dependency Message
     result.messages
       .filter((msg) => msg.type === 'dependency' ? msg : '')
       .forEach((file) => watcher.add(file))
   })
})

Test && Fix

@RyanZim You mentioned node-suppose for CLI testing in #44. Let's go with a AVA/Suppose combo for testing 😛

Could you create a e.g develop branch please?

RyanZim commented 7 years ago

@michael-ciniawsky Some good points.

Could you create a e.g develop branch please?

I'd like to make sure @ai is OK with switching to postcssrc-cli as the main codebase before starting work on this. @ai are you OK with moving forward in this direction?

I disagree on a few of your suggestions above, but let's wait to bikeshed about them until we get official word from @ai. (Threads in the postcss org tend to get long & noisy :wink: )

ai commented 7 years ago

I like this 3 steps — they are very important. But don’t forget to some help for migration. Maybe config migration tool?

I am OK with rewritting code from scratch/ Sometimes it is the best solution :).

RyanZim commented 7 years ago

I am OK with rewritting code from scratch/ Sometimes it is the best solution :).

:+1: I think this is one of those cases.

But don’t forget to some help for migration.

Yeah, will definitely add at least a detailed migration guide; not sure about a config tool yet, we'll see.

@michael-ciniawsky Will try and get a develop branch asap.

RyanZim commented 7 years ago

@michael-ciniawsky A couple things:

I'm not totally sold on streams yet. They might be a good idea, but let's get v3 shipped ASAP without streams. Then we can revisit this for a later version.

Needs a 90% finished trivial update to expose the config.file being able to watch the config for changes aswell, when --watch|-w is enabled.

:+1: See https://github.com/postcss/postcss-cli/issues/26.

--use|-u or --ctx|--config|-c should support setting the context from the command line to override options set via ctx.? in postcss.config.js

Yep, we should support setting the ctx from the CLI. Not sure what the flag should be called, or what the usage should be.

Strawman proposal:

postcss --ctx {sugar: true, env: 'development'}

--config|-c should scaffold a postcss.config.js file if missing, alternatively plugins/options set via '--use|-ushould create apostcss.config.js` file if none present.

Not a bad idea to support creating a postcss.config.js file from the --use data. This shouldn't happen by default, though. How about having a flag --create-config?

Of course, we need to add support for --use first.

ai commented 7 years ago

Yeap, migration guide will be s solution too.

RyanZim commented 7 years ago

@michael-ciniawsky @ai Branch is here: https://github.com/postcss/postcss-cli/tree/develop.

Please look over it and point out any mistakes I made (if forgot to change postcssrc-cli to postcss-cli somewhere, etc.).

ai commented 7 years ago

Sorry, I ma in the middle of installing new system :).

michael-ciniawsky commented 7 years ago

I'm not totally sold on streams yet. They might be a good idea, but let's get v3 shipped ASAP without streams. Then we can revisit this for a later version.

Shouldn't be to difficult to implement though and process.stdout/process.stderr smells streamy anyways, so this could simplify the codebase imho. Agreed that it's nice-to-have and not mandatory atm.

Not a bad idea to support creating a postcss.config.js file from the --use data. This shouldn't happen by default, though. How about having a flag --create-config?

@RyanZim Yep we need an init module for the CLI, primary for CLI users and as convenience method for scaffolding/updating a config in general.

lib/config.js

// ? :)

postcss --init|-i [path/to/config || process.cwd()] [--rc|--pkg] [--legacy|--migrate oldConfig.json]

Not a bad idea to support creating a postcss.config.js file from the --use data. This shouldn't happen by default, though. How about having a flag --create-config?

Well if no postcss.config.js found, the CLI creates one from a template + use data + defaults, if one is present the use data is passed as ctx and postcss-load-config does the job it does.

Of course, we need to add support for --use first.

Of course, I removed it because of everything mentioned in this post, starting from scratch to see whats possible, but no time to finish it 😛

Yep, we should support setting the ctx from the CLI. Not sure what the flag should be called, or what the usage should be.

Yeah thats pending atm, it has some overlaps with --use|-u, but isn't a complete replacement either.

Strawman proposal: postcss --ctx {sugar: true, env: 'development'}

Context

index.js


if (argv.env) process.env.NODE_ENV = argv.env

if (config) {
 // Update context 'store'
  ctx = {
    parser: argv.parser ? argv.parser : false
    map: argv.map ? argv.map :  ('inline' || false) // Should be false imho :D
    plugins: {...argv.use}
  }
} else {
  init(argv) // Create config 
}

postcss --env|-e 'production' postcss --map|-m 'inline' postcss --use|-u plugin option=value, plugin option=value.... postcss --parser|-p 'sugarss' etc..

But still unfinished...

Branch is here: https://github.com/postcss/postcss-cli/tree/develop.

👍

RyanZim commented 7 years ago

I know we did this for postcss-import, but I'm not sure if discussing everything related to a new release in a single issue is a good idea. We end up with a terribly long disorganized thread.

I've created a GitHub Project for v3, outlining most of what needs done. If I missed something that needs done, please open an issue, & I will add the issue to the appropriate column. Project is here: https://github.com/postcss/postcss-cli/projects/1

Notes that need discussion will be made into issues.

If you have a brainstorm, please open a separate issue for discussing it.

I realize this is a fairly new approach, but I think it will keep us all more organized. I'm going to close this issue, discussion about setting context via command line can continue at #52.

postcss / postcss-cli