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

Don't set the "from" option when reading from stdin. #122

Closed
ben-eb opened this issue Apr 23, 2017 · 16 comments
Closed

Don't set the "from" option when reading from stdin. #122

ben-eb opened this issue Apr 23, 2017 · 16 comments

Comments

@ben-eb
Copy link
Member

ben-eb commented Apr 23, 2017

The from option indicates the path of the file being read from; in the case of stdin, it's a special case that I think we have to handle differently.

With the current behaviour, because I am using cosmiconfig to find configuration presets, an error is thrown because https://github.com/postcss/postcss-cli/blob/master/index.js#L287 (/path/to/project/stdin) is not a valid file. I suggest that we change this behaviour to set this option only when file does not equal stdin; e.g:

if (file !== 'stdin') {
    options.from = file;
}

Replacing the problematic line with this above code resolves the issue for me.

@michael-ciniawsky
Copy link
Contributor

michael-ciniawsky commented Apr 23, 2017

if I remember right, the dummy path/to/stdin.css path was added, bc of browserlist (autoprefixer) otherwise fails to lookup the config (!options.from) and it wouldn't work with process.stdin then.

@ben-eb
Copy link
Member Author

ben-eb commented Apr 23, 2017

OK. Then why not:

options.from = file === 'stdin' ? process.cwd() : file;

@michael-ciniawsky
Copy link
Contributor

I asked the same 😛 => #114

Seems options.from expects a 'full path' [dirname]/[basename]/[extname] but I didn't check it tbh :D

@ben-eb
Copy link
Member Author

ben-eb commented Apr 24, 2017

So as I see it, my options are:

  1. Recommend people to not use postcss-cli with stdin - bad, as it confuses users.
  2. Carry on recommending people to use cssnano-cli instead - bad, fragmentation & more maintenance.
  3. Write a dirty hack in cssnano to accommodate this use case - bad, it handles only my use case & not future use cases.

I don't want to do any of these and would prefer the removal of the fake file; there has to be a better solution. If that's not changed I guess I will be sticking with option 1 as it is the least bad one.

@RyanZim
Copy link
Collaborator

RyanZim commented Apr 24, 2017

See #115 (comment)

[I] will revert if this causes other issues.

It seems it does. @ai Thoughts here?

@michael-ciniawsky
Copy link
Contributor

michael-ciniawsky commented Apr 24, 2017

I don't want to do any of these and would prefer the removal of the fake file; there has to be a better solution. If that's not changed I guess I will be sticking with option 1 as it is the least bad one.

Solution Prototype

In General 👍, it's a 'bug' in browerslist 😛

Recommend people to not use postcss-cli with stdin - bad, as it confuses users

Disagreeing here 😛 A 'classical/traditional' CLI has to support STDIN/STDOUT and Unix Pipes, it may be not the most used, but that's where PostCSS support for this is/should be located.

Carry on recommending people to use cssnano-cli instead - bad, fragmentation & more maintenance.

Is it 'confirmed' that cosmiconfig is the source of the bug ? Why not postcss.config.js especially in terms of

bad, fragmentation & more maintenance.

? 😉 , but I see the use of .cssnanorc aswell , cssnano is often used standalone.
Could the plugin use postcss.config.js and only the wrapper uses .cssnanorc?

@ben-eb
Copy link
Member Author

ben-eb commented Apr 24, 2017

I am using cosmiconfig to find a config file relative to the input, the problem is that it performs a stat call on the file path (/path/to/project/stdin) and errors out because the file does not exist. So this error is coming from cosmiconfig:

$ echo "h1 { color: black }" | postcss
✖ Processing stdin
{ Error: ENOENT: no such file or directory, stat '/path/to/project/stdin'
    at Error (native)
  errno: -2,
  code: 'ENOENT',
  syscall: 'stat',
  path: '/path/to/project/stdin' }

I can resolve this by removing the cosmiconfig call, so I figure it's the source of the error.

Ideally I want to deprecate cssnano wrappers so that I have less to maintain, am happy if people want to take up that workload but I won't be doing so. Possibly should create a separate issue for this. And cssnano v4 ships with presets, it's different to postcss.config.js.

@ai
Copy link
Member

ai commented Apr 24, 2017

Maybe we need to fix cosmiconfig to not throw on unknown file?

@ben-eb
Copy link
Member Author

ben-eb commented Apr 24, 2017

I guess cosmiconfig needs to stat the file path to check if it's a directory or not. https://github.com/davidtheclark/cosmiconfig/blob/de81cf6f4bd8f18f4c36255b8a9f13733e3cc7f7/lib/createExplorer.js#L100

@michael-ciniawsky
Copy link
Contributor

michael-ciniawsky commented Apr 24, 2017

? 🙃 Use a process.cwd() fallback in your cssnanorc implementation, the same would be good for browserslist aswell, both introduce possible side effects (impure) for other tools/runners/implementations atm.

And cssnano v4 ships with presets, it's different to postcss.config.js

Yep, but by design choice then 😛 and why ? Will it be incompatible to postcss.config.js in general ?

postcss.config.js

module.exports = () => ({
  plugins: {
    cssnano: { preset: 'name', [presetOptions: {...}] } // ?
})

@ben-eb
Copy link
Member Author

ben-eb commented Apr 24, 2017

Like I said, it's a dirty hack. I should expect that the file coming from from is an actual file, I shouldn't have to analyse the path for a mention of stdin and replace it with process.cwd(). What about the fact that postcss-cli introduces this side effect in the first place?

No, you can pass the preset directly as an option to cssnano, so it's not incompatible. 😄

@ben-eb
Copy link
Member Author

ben-eb commented Apr 24, 2017

By the way it would probably be better to use a file abstraction rather than making up paths on disk, such as https://github.com/vfile/vfile or https://github.com/gulpjs/vinyl.

@michael-ciniawsky
Copy link
Contributor

The responsibility to handle this impure behaviour was basically shifted form browserslist => postcss-cli, which I was skeptical about from the beginning and therefore postcss-cli has inherited the possible side effects (impurity) now. Your is q.e.d for my assumption 😛

@michael-ciniawsky
Copy link
Contributor

By the way it would probably be better to use a file abstraction rather than making up paths on disk, such as https://github.com/vfile/vfile or https://github.com/gulpjs/vinyl.

No extra deps 💪 😛 to handle architectural issues, if solvable otherwise

I suggested to use yargs, glob-stream, glob-watcher, vinyl + a PostCSS transform stream (basically a cleanupped gulp-postcss) to have a cleaner interface between File I/O and process.stdin/process.stdout but it was rejected 😛

@ben-eb
Copy link
Member Author

ben-eb commented Apr 24, 2017

¯_(ツ)_/¯

@ben-eb
Copy link
Member Author

ben-eb commented Apr 26, 2017

For me this is resolved with the latest cosmiconfig, so I don't think we need a resolution here for now. 😄

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

No branches or pull requests

4 participants