callstack / linaria

Zero-runtime CSS in JS library
https://linaria.dev
MIT License
11.64k stars 416 forks source link

Not compatible with Webpack file and url loaders? #159

Closed steadicat closed 6 years ago

steadicat commented 6 years ago

Do you want to request a feature or report a bug? Bug.

What is the current behavior? Webpack’s file-loader or url-loader (they come standard with create-react-app) let you require() (or import) any file type (like images and fonts). At build time, the require statement gets replaced with a URL that you can use to load the file at runtime. Unfortunately Linaria seems to break any time I reference anything that gets loaded this way. It appears to attempt to load the file as if it were a JS module. E.g. both of these fail:

css`background-image: url(`require('image.svg')`);`
css`@font-face { font-family: Test; src: url(${require('webfont.woff')})}`;

If the current behavior is a bug, please provide the steps to reproduce and a minimal repository on GitHub that we can yarn install and yarn test. Quick create-react-app that attempts to load an SVG and a web font on the home page: https://github.com/steadicat/linaria-test

What is the expected behavior? Expected behavior is for Linaria to let the loader run first so that those requires become plains strings to be included in the CSS.

Please provide your exact Babel configuration and mention your Linaria, Node, Yarn/npm version and operating system. See app above.

satya164 commented 6 years ago

You need to make sure that these require statements are processed before Linaria gets to them. Maybe you can move the file-loader and url-loader to the top so that they run before babel-loader runs (and also processes JS files) and it should be fine. Lemme know if this works.

steadicat commented 6 years ago

The url-loader in create-react-app is already before babel-loader. I tried moving it to the very top, and adding enforce: 'pre', still no luck. My assumption (based on zero knowledge of Linaria internals) was that whatever magic Linaria is doing to to resolve modules at compile time in order to expand interpolated strings is jumping the gun and running before Webpack has a chance to let the other loaders run.

satya164 commented 6 years ago

Linaria evaluates the code to resolve the requires using node's semantics and webpack doesn't have a chance to interfere. So the only way to support it would be to process these before linaria runs. Interesting that moving the loaders doesn't fix it.

cc @zamotany do you have any idea why it happens?

steadicat commented 6 years ago

As I understand it, there is no way in Webpack to “process the files before Linaria runs”. Files are loaded one by one as they are encountered during parsing. Loaders run in order on each file, not in order on the whole codebase. So if Linaria reads from any file other than the one currently being parsed, it's jumping the gun and not letting Webpack process the other files beforehand as it should.

Perhaps there's a way to call back into Webpack to let it do its thing? Or to wait until all the dependencies have been preprocessed before attempting to load and run them?

satya164 commented 6 years ago

it's jumping the gun and not letting Webpack process the other files beforehand as it should

As you said, files are loaded one by one. So there's no way for Linaria to wait for all files to get processed. Note that Linaria is just a babel plugin, it doesn't know about webpack. Since Linaria extracts the CSS out, the file must be processed before Babel runs on it.

As per individual files, as long as babel-loader runs after other loaders such as url-loader and file-loader run, it should work fine. Not sure why it doesn't work.

@zamotany is probably more familiar with it and might be able to provide some insight.

steadicat commented 6 years ago

Correct me if I’m wrong, but Linaria needs to import the dependencies of the current file in order to run the code that's interpolated in the CSS, right? At that point, the current file’s dependencies have not yet been processed by Webpack, as the references have just been encountered. If my assumptions are correct, it seems pretty clear why it doesn’t work. In fact, I don’t see how it can work. Short of rewriting it as a Webpack plugin.

satya164 commented 6 years ago

Linaria needs to import the dependencies of the current file in order to run the code that's interpolated in the CSS, right? At that point, the current file’s dependencies have not yet been processed by Webpack

Yes. But only if those dependencies are used inside the tagged template literals. It won't try to import dependencies which are not used.

e.g. -

import MyComponent from './MyComponent';
import colors from './colors';

const title = css`
  color: ${colors.blue};
`;

It won't try to import MyComponent since it's not used inside the template literal, but it will import colors.

steadicat commented 6 years ago

Right. And in my example, the problem arises when trying to reference images or fonts inside the css`` string. Makes sense.

Curious to hear what @zamotany thinks, but it sounds like this is not going to be doable.

FWIW, it seems to me that Linaria is bending the rules a little bit. I don't think Webpack loaders (or Babel plugins) are supposed to go out and read their own dependencies straight off of the filesystem. Doesn’t this break any preprocessing that might be done by other plugins, or any custom module resolution logic?

Have you guys thought about using a Webpack plugin instead?

satya164 commented 6 years ago

FWIW, it seems to me that Linaria is bending the rules a little bit. I don't think Webpack loaders (or Babel plugins) are supposed to go out and read their own dependencies straight off of the filesystem.

It doesn't read dependencies from the filesystem. It evaluates the code in node, and any babel plugins you use will still apply.

Doesn’t this break any preprocessing that might be done by other plugins, or any custom module resolution logic?

Yes, custom module resolution logic will break unless you do it with a babel plugin like babel-plugin-module-resolver.

but it sounds like this is not going to be doable.

I'm not sure why this doesn't work

Say this is your code

const cover = css`
  background: url(${require('./image.png'});
`;

It should get processed by url-loader and become

const cover = css`
  background: url(static/image.png);
`;

Then it should get processed by babel-loader when Linaria can extract the static CSS.

Have you guys thought about using a Webpack plugin instead?

I don't see how this can be achieved with an webpack plugin

A quick fix that might take care of my use case would be for Linaria to simply ignore any require or import that references a non-JS file. Just treat the statement as a constant (which it will be eventually after the other loaders have run).

No, that won't work. The CSS is extracted out and not left in the JS. The value of the constant must be known when the CSS gets extracted by Linaria.

But probably this will work:

Say this is your code:

const cover = css`
  background: url(${require('./image.png'});
`;

When Linaria sees a require for a non-js file, it could replace it with the string value when generating the CSS:

._cover__653hgd {
  background: url('./image.png');
}

Then this gets processed by url-loader as usual.

steadicat commented 6 years ago

I'm not sure why this doesn't work

You’re right, it should work, at least in most cases. I can still imagine cases where the current file is referenced by css literals in other files, in which case Linaria has to evaluate it before Webpack does its thing. That's gonna be really hard, if not impossible to fix, I imagine.

Then this gets processed by url-loader as usual.

I like this idea. It hadn't occurred to me that url-loader also works on CSS files.

satya164 commented 6 years ago

You could probably try changing this line https://github.com/callstack/linaria/blob/master/src/babel/lib/moduleSystem.js#L161 to return moduleId.includes('.') && !moduleId.endsWith('.js') ? moduleId : getRequireMock(this)(moduleId); if you want to give it a go

steadicat commented 6 years ago

I think I just stumbled upon the answer to the “why it doesn’t work” question. Here’s what the raw JS looks like after url-loader does its thing:

/***/ "./src/logo.svg":
/*!**********************!*\
  !*** ./src/logo.svg ***!
  \**********************/
/*! no static exports found */
/*! all exports used */
/***/ (function(module, exports, __webpack_require__) {

module.exports = __webpack_require__.p + "static/media/logo.5d5d9eef.svg";

/***/ }),

/* [...SNIP...] */

var logo = __webpack_require__(/*! ./logo.svg */ "./src/logo.svg");

It appears that url-loader does not replace the require statement with a string, it actually creates a dummy module that exports a string. I don’t see how Linaria can work around that. 😞

zamotany commented 6 years ago

So here's the thing: Linaria implement module system similar to node's one, thus it only supports .js files (in future .json and .node). Requiring binary/text-like files (.svg, .woff, .png, ...) is a custom solution specific only to webpack with implementation details living inside source code of specific loader.

That being said, supporting it by returning a file path is doable, however it will become problematic due to webpack's output.publicPath (internally __webpack_require__.p).

So currently there's no way of using file/url loaders with linaria. We will try to came up with some solution, but since we want to be bundler-agnostic it might take some time.

satya164 commented 6 years ago

@zamotany shouldn't the publicPath stuff be taken care of by url-loader when we have url(./path/to/file) in the css code? Though I don't know since I haven't tried.

zamotany commented 6 years ago

@satya164 this is not how it works, even when url-loader/file-loader runs, they don't modify original require, they create a module that returns correct path with publicPath and there is not way for linaria to get to that module. We would need to reimplement the whole url-loader/file-loader logic and get publicPath from somewhere like preset options.

satya164 commented 6 years ago

@zamotany yes, but I'm not talking about JS. what happens when url-loader processes a CSS file with url(./path)?

zamotany commented 6 years ago

Usually you won't be processing CSS files with url-loader, tho if you do it depends on options and the content of the file - if file size is smaller that limit (from options) or there is no limit it will be inlined as data:base64 otherwise it will be passed to fallback loader (by default file-loader), which will create a dummy module with public path for that file.

Answering your @satya164 question: shouldn't the publicPath stuff be taken care of by url-loader when we have url(./path/to/file) in the css code? - maybe.

satya164 commented 6 years ago

@zamotany as long as we have css-loader setup (which we would if we're using linaria with webpack), these should already be handled since css-loader interprets @import and url() like import/require() and will resolve them. so url-loader should work with them. am I missing something?

@steadicat ofc you should be able to just write url() instead of require and let css-loader handle it, then linaria won't need to care about it, right?

zamotany commented 6 years ago

Special case in linaria to create a dummy module that should export url('...')

satya164 commented 6 years ago

I'm referring to this line in css-loader README - https://github.com/webpack-contrib/css-loader#usage, which mentions that loaders like file-loader and url-loader work with it.

zamotany commented 6 years ago

Still, we need to replace require('file.ext') with url('file.ext') in final css and it needs to be done by linaria.

satya164 commented 6 years ago

@zamotany in the code we write url(${require('image.png')), so it just needs to be replaced with the module string to get url('image.png'). So it should be enough to return the received argument from the require method.

zamotany commented 6 years ago

@satya164 In that case, yes

steadicat commented 6 years ago

The solution of using plain CSS url() works for me. There are two actionable things:

  1. Document this more clearly. I saw a mention of this at the bottom of this file. Would be great if this was in the README.
  2. Fix the paths. Currently url()s referenced inside Linaria CSS are relative to the location of the Linaria generated CSS, which is awkward. It would be great if the paths got rewritten to be relative to the current file.