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

Load upstream source content from the filesystem #1346

Open
nex3 opened this issue Apr 13, 2020 · 3 comments
Open

Load upstream source content from the filesystem #1346

nex3 opened this issue Apr 13, 2020 · 3 comments
Labels

Comments

@nex3
Copy link
Contributor

nex3 commented Apr 13, 2020

Currently, PostCSS never tries to load a source file's contents. This means that error messages won't display the upstream file's contents unless they're included in sourcesContents, which for large compilations could be extremely large.

To reproduce:

// test.js
const postcss = require('postcss');
const fs = require('fs');

const root = postcss.parse(
  fs.readFileSync('test.css', 'utf8'), {from: 'test.css'});
console.log(root.nodes[0].error("oh no").toString());
$ echo 'a {b: c}' > test.scss
$ sass test.scss test.css
$ node test.js
CssSyntaxError: /home/nweiz/goog/postcss/test.scss:1:0: oh no

Compare to:

$ echo 'a {b: c}' > test.scss
$ sass --embed-sources test.scss test.css # Requires Dart Sass
$ node test.js
CssSyntaxError: /home/nweiz/goog/postcss/test.scss:1:0: oh no

> 1 | a {b: c}
    |                                      ^
  2 | 
@ai
Copy link
Member

ai commented Apr 13, 2020

Are you want to add real source code according to source map into CssSyntaxError message?

There are a few questions, which we should answer first:

  1. What if the previous tool broke CSS? By loading source, we will miss that broken output.
  2. What is use case? I try to keep PostCSS core away from file system, because it could be run in many different environments. Current policy is that PostCSS runner (postcss-loader, gulp-postcss) should deal with it and it works for most of the case. But I understand that some use case could not fit the policy and we can find workaround for it.

@nex3
Copy link
Contributor Author

nex3 commented Apr 13, 2020

Are you want to add real source code according to source map into CssSyntaxError message?

Yes. This is already the default behavior if "sourcesContent" exists, as I demonstrated in my second example above—I just want it to work when "sourcesContent" is not set as well.

There are a few questions, which we should answer first:

  1. What if the previous tool broke CSS? By loading source, we will miss that broken output.

Right now:

  • If "sourcesContent" is set, the error will show the original source code, not the intermediate CSS that's being parsed by PostCSS.

  • If "sourcesContent" is not set, the error will not show the original source code at all.

That's definitely not useful or consistent behavior. I think the question of whether the user wants to see the original source code or the generated CSS and how to configure that is separate from this issue (although currently PostCSS defaults to showing the original source code which I think is most likely to be useful).

  1. What is use case?

My particular use-case is integrating PostCSS into Google's build system, but I think having PostCSS correctly parse source maps by default is generally valuable.

I try to keep PostCSS core away from file system, because it could be run in many different environments. Current policy is that PostCSS runner (postcss-loader, gulp-postcss) should deal with it and it works for most of the case. But I understand that some use case could not fit the policy and we can find workaround for it.

The source map handling already bends this policy by loading a source map from a CSS file's sourceMapURL annotation by default. I think it's consistent with that behavior to have it also try to load the upstream files if "sourcesContent" isn't available.

I think PostCSS's system of having reasonable default behavior of loading stuff from the filesystem, but having that be overridable using the map.prev option, makes a lot of sense. The problem here is that the default behavior isn't complete enough.

@ai ai added the feature label Jul 22, 2020
@ai
Copy link
Member

ai commented Jul 22, 2020

I think I will move this issue for 8.1+.

A pull request is welcome to release it faster.

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

No branches or pull requests

2 participants