[WIP, please help improve] Fix #699 when importing css using postcss

[Evertt]

All the bug info can be found in #699, but here's a short summary.

When you use import './styles.css' in your Svelte or JS code and you use rollup-plugin-postcss to process that css file first, then your css output looks like javascript. Depending on your configuration it could look like:

export default undefined;
export default undefined;
/* sourceMappingURL=./chunk.e3537e20.css.map */

or

var css = "a.link.svelte-131civd{color:#d3d3d3}";
export default css;
import styleInject from 'C:/Users/admin/Code/domainhax/node_modules/style-inject/dist/style-inject.es.js';
styleInject(css);
var css = "main.svelte-1joomdn{padding-right:15px;padding-left:15px;margin-right:auto;margin-left:auto;width:64em}input.svelte-1joomdn{width:100%}#description.svelte-1joomdn>div.svelte-1joomdn{width:50%;padding:0;float:left}header.svelte-1joomdn{text-align:center}p.svelte-1joomdn{margin-top:0}a.svelte-1joomdn{color:#959595}h1.svelte-1joomdn a.svelte-1joomdn{color:#dfdfdf}";
export default css;
import styleInject from 'C:/Users/admin/Code/domainhax/node_modules/style-inject/dist/style-inject.es.js';
styleInject(css);
/* sourceMappingURL=./chunk.e3537e20.css.map */

I saw that the bug has gone unfixed for a long time. Now I don't understand Sapper well enough to understand all the intricacies that I need to be mindful of when fixing a bug. So I'm aware that this PR is probably relatively bad. I didn't run the tests, but when I run npm run dev I get the following warning repeatedly:

Sourcemap is likely to be incorrect: a plugin (sapper-internal) was used to transform files, but didn't generate a sourcemap for the transformation. Consult the plugin documentation for help

But other than that, it now does output the css correctly, finally. So is anyone willing to take a look at my PR and improve whatever needs to be improved before it can be merged?

[benmccann]

There's a lot to comprehend regarding this bug and I'm not sure I totally understand it yet because I'm not that familiar with PostCSS, etc. Looking for styleInject seems too specific to this usecase though and I think it'd be better handled in a more generic way. The first line of this function is checking to make sure that the code is CSS. Perhaps the issue here is that you actually have JavaScript in a file with a .css extension? It seems weird to me that you would have that, but perhaps the fix should be to check if the file contents are JavaScript instead of CSS? But I'm not quite sure a good way of doing that. Looking for something like export seems a bit better, but also seems like it could cause problems. E.g. what if the user has some data export functionality on their site and has a CSS class named .export?

[benmccann]

What's the difference between return; like on the first line and return code; that you have here?

[Evertt]

In this case I'm just mimicking the code that was there previously. That also either returned nothing (if the id did not test positive for /\.css$/) or it returned an empty string.

[benmccann]

Can you explain why you went with this approach instead of the one suggested by @krzysztof-grzybek in #699 (comment)?

We should also test that any solution would work with code splitting. Code splitting currently only works with Rollup 1. We should probably merge #1306, which I sent yesterday and touches this same block of code, to fix code splitting support for Rollup 2 to make it easier to test since a lot of the sample apps use Rollup 2

[Evertt]

Can you explain why you went with this approach instead of the one suggested by @krzysztof-grzybek in #699 (comment)?

Simply because I don't understand it enough to be able to implement it. I tried to play a little bit with the writeBundle hook, but I just got more errors that I didn't understand.

We should also test that any solution would work with code splitting. Code splitting currently only works with Rollup 1. We should probably merge #1306, which I sent yesterday and touches this same block of code, to fix code splitting support for Rollup 2 to make it easier to test since a lot of the sample apps use Rollup 2

👍

[benmccann]

I appreciate you sharing your findings. I actually think it'd be more helpful to close this PR though. We have a very long review queue and it makes it harder to know which PRs need review when we have PRs that aren't in a mergable state in the review queue

[Evertt]

👍