tivac / modular-css

CSS Modules, but better and usable via Rollup, Vite, Webpack, CLI, PostCSS, or JS API
http://m-css.com
MIT License
288 stars 19 forks source link

Processor.file() does not walk CSS dependencies if they are `@import`ed #821

Closed ivanjonas closed 2 years ago

ivanjonas commented 2 years ago

Expected Behavior

@modular-css's processor.js has a file function that adds a file to the Processor instance. This part works well.

const extraStyles = path.resolve(aliases.assets, "styles/extraStyles.css");
await processor.file(extraStyles);

The extraStyles.css file in question is simple:

@import "./element.css";

And that file, in turn, contains only a basic CSS rule.

body { background: red; }

I expect that when I consume the processor with @modular-css/rollup in watch mode, saving either the extraStyles.css or the element.css files will trigger a rebuild.

Current Behavior

Rollup will only rebuild extraStyles.css on save. ~Curiously, saving element.css will cause the terminal to produce this single line, but will not trigger a rebuild:~ (this detail was from a custom file watcher in our project, ignore)

If I change extraStyles.css to be as follows, the dependency walking logic does pick up element.css and call this.addWatchFile on it:

@value * from './element.css';
@import './element.css';

Possible Solution

Not a solution, but I've done some source code diving and found a few things. First, the _walk method that is used by Process.file does not recognize any CSS @imports as dependencies. Second, @modular-css\processor\plugins\before\graph-nodes.js calls some PostCSS APIs to walkAtRules for @value and @composes, but not @import. Perhaps relevant?

I suspect there may be another means by which to make Rollup watch every @imported dependency that is different from my current approach. What do you think?

Steps to Reproduce (for bugs)

Not sure if this is a bug or a feature request or just a request for guidance; please advise before I spin up a demo.

Context

My application uses Rollup and the Svelte m-css preprocessor. The above TypeScript is run in a tiny custom plugin that runs before both rollup-plugin-svelte and @modular-css/rollup. The bottom line goal is to append more CSS to the generated Rollup bundles, CSS that isn't referenced anywhere in the remainder of the app (complex app requires dependency inversion, so the bulk of the app cannot reference extraStyles.css directly. More details can be provided if needed).

Your Environment

Executable Version
modular-css 25.8.2
npm --version 6.14.6
node --version 14.19.0
OS Version
Windows 10 1607
tivac commented 2 years ago

modular-css has no built-in concept of @import, the most common way to inline @import statements would be to use postcss-import in either the before or after hook. That still won't get rollup to watch it for you, but it looks like it does output a message with the paths of the files that were imported, so you might be able to get access to that somehow?

If you're manually adding files to a processor instance before passing it as the processor param then those files will be watched:

https://github.com/tivac/modular-css/blob/90370d2b7007deb078d3acd2716b0a35a98f6c78/packages/rollup/rollup.js#L90-L91

But unfortunately since the @import is purposefully not handled by modular-css it won't be part of the overall dependency graph and thus not watched for changes.

I'd be open to a PR adding support for checking for messages like type: "dependency" and reporting those outward, you'd probably want to make sure you have access to the messages array on this line

https://github.com/tivac/modular-css/blob/90370d2b7007deb078d3acd2716b0a35a98f6c78/packages/css-to-js/css-to-js.js#L59

and then make sure any of the dependency messages from postcss-import get added to the dependencies Set kinda like these ones:

https://github.com/tivac/modular-css/blob/90370d2b7007deb078d3acd2716b0a35a98f6c78/packages/css-to-js/css-to-js.js#L65-L73

And then those dependencies will bubble all the way out to rollup and get added as watched files.

ivanjonas commented 2 years ago

Thanks for the brainstorm. I'm wondering if a simpler approach for my particular situation wouldn't be to simply watch all .css files recursively from a single directory. It's not perfectly efficient (what if there are unused files in that directory that we don't care to watch?) but it might be the pragmatic answer.

I'm considering this PR in any case. I've poked around in the source code a bit, so it might be within reach. But a question first: why the decision for modular-css to not have a concept of @import? It seems like that would be a first-class way to parse dependency trees. Would introducing the concept to m-css create a conflict in its philosophy?

tivac commented 2 years ago

modular-css is an implementation of the CSS Modules Spec which doesn't call for any special handling of @import. I've incorporated a few proposals that aren't part of the spec because they seemed valuable and it wasn't something that could be easily be supported by a plugin that didn't have access to some of the internal data in the m-css processor.

But @import is fairly standalone, handled well by postcss-import, and that plugin is way more feature-rich than any implementation I would make.

That said, I did some digging and the type: "dependency" messaging is actually a postcss convention/standard for runners, so support for that should absolutely be added to m-css.

ivanjonas commented 2 years ago

Neat, I'm learning a lot, thanks for sharing!

ivanjonas commented 2 years ago

My problem statement is morphing a little bit, and I'm sorry for the churn. The original post cited file watching (something we can adequately solve with a tiny custom Rollup plugin), but the root of that problem is also creating a bigger issue for us. Specifically, we need the "namer" function to be able to calculate a scoped class name based on the file's path.

I've looked at just using postcss-import as you recommend (and it does seem robust), but the shortcoming with that approach for our specific use case is that all of the @imported dependencies are inlined into the root file. That means that the "namer" function loses the ability to name things based on the @imported file's path. This is a critical point of our admittedly unusual needs. All it can do at that point is name things based on the root file's path, which is not helpful. With that new information, do you have any suggestions for me to try?

Aside: I'm still surprised and conflicted that an @imported CSS source file isn't considered a dependency. Doesn't seem like special handling as much as the basic definition of a dependency. But I can easily imagine how m-css might create conflicts with postcss-import if it did try to do something unique with imports. At best, it might not even work, since postcss-import often runs at the beginning of the pipeline. Those @import statements might not even exist by the time m-css gets to process the files. So don't worry, I'm not even entertaining the notion of asking for that as a feature.

tivac commented 2 years ago

I'm gonna see if I can restate this to be sure I understand:

You want a.css to have a dependency on b.css, without actually depending on any of the classes or @values in that b.css, and for b.css to be correctly scoped via the namer function, which prevents you from using @import and postcss-import to inline b.css into a.css?

I feel like I'm missing something still. What is in b.css that a.css needs, but isn't actually part of m-css?

If I have the question summarized correctly above, I think you may be able to have the file that is actually doing import "./a.css"; also import "./b.css";. That would take care of including b.css in the output and the namer scoping issues, though you may need to name the file something like _b.css to abuse the sorting a bit since there wouldn't be an actual dependency linkage between the two.

ivanjonas commented 2 years ago

Very close, and your idea does give me a promising new idea. In the meanwhile, here's a full explanation of the unusual situation, if you are interested.

We are building an app that has standard styles, but will be adopted by many users and can be arbitrarily custom-styled as those users deem necessary. These new styles cannot be imported by the Svelte app via javascript because the application does not know in advance what source files a user may or may not have created.

Start with normal stuff: a typical Svelte app, with its own modular-css source files that are imported from .svelte files. All that css gets ultimately output as a single css file, "application.css". An example of these source files might be /src/components/heading/styles.css, and it might have a single class .title that is processed into .components_heading_title by the namer function.

Here's the novel stuff: I wand a second output file, "extraStyles.css", to be generated by Rollup and ingested by the running application. Rules in this file will extend and override rules in application.css. This file contains the processed output of several user-created css source files, and the svelte app has no idea what they might be. An example of these user-created source files might be /src/extras/components/heading/styles.css, and it would have another .title that is also processed into .components_heading_title by the namer function.

Key in making this scheme work (and emoji to denote what I've been able to accomplish so far):

Helpful in making this scheme work well:

/* extraStyles.css */

@value * from "./components/heading/styles.css";
@import       "./components/heading/styles.css";

Hoping you aren't going "gah I can't believe these shenanigans" right now. I very much appreciate your time, expertise, and suggestions, and I am trying to achieve these things without creating more work for either of us.

tivac commented 2 years ago

Questions:

1) Are the files in extras/ are available at build time? Or is the plan for them to be bundled later as part of a separate process?

Thoughts:

1) Are you expecting the users providing the overrides in extras/**/*.css to be using m-css features, or mostly just overriding classes? If you don't expect them to use m-css features it feels like treating CSS files in extras/ as a special class of file and using something simpler like postcss with a custom plugin to scope class selectors might be easier for your purposes? I'm obviously biased but postcss plugins aren't terribly difficult to create and so long as you can put some constraints on the input CSS walking every classn and transforming it is not a complex plugin. 2) Could you use something like import from "./extra/**/*.css"; in rollup (using a plugin that adds import globbing support) and sidestep some of these issues? I guess you'd need to be careful about ordering in the resulting bundled CSS, but if all those files are leaf nodes (because they don't have any dependencies on any other files) then in theory they should naturally filter towards the end of the bundle and be fine. 3) m-css should respect the dependency messages, thought it won't solve this because you'd still have the @import namer args issue.

tivac commented 2 years ago

Another question:

Why do folks have to match your directory structure to override styles? If your namer already produces unique but friendly names I don't see why forcing themes to match your particular file structure helps them.

It really seems like you if you @import all the css files in the extras dir, and we fix dependency message handling, it'd be simpler for everybody.

ivanjonas commented 2 years ago

It really seems like you if you @import all the css files in the extras dir, and we fix dependency message handling, it'd be simpler for everybody.

Oh yes, if @imported files were treated as dependencies, it truly would solve everything on my end.

Answer to your other thoughts:

  1. We do want users to be able to use @value in their extra stylesheets. But that's a good thought... if we can remove that need (with, say custom properties), then we could just go without modular-css.
  2. This is a great idea, my only concern is the ordering, which would have been explicitly second in a separate css file. I will experiment with this approach, too.
  3. Agreed, so please don't worry about implementing this just for my sake.

Why do folks have to match your directory structure to override styles?

Correct, and a good point raised by another teammate. Just because the framework uses the tree structure to have modular-css namer generate the friendly class names doesn't mean that users can't simply manually write those friendly class names and ditch the identical file structure. But the problem would still remain: users would likely want a bunch of small, logically separated CSS files that are @included by extraStyles.css.

OK, let me try that import from "./extra/**/*.css"; approach first. Could be one-and-done.

ivanjonas commented 2 years ago

I forgot to answer your very first question. Yes, all the files in extras/ are known at build time.

ivanjonas commented 2 years ago

Coming back to leave an update. In the end, I sidestepped all of these challenges and went another route.

I processed the extra files into their own bundle of CSS so now there are two CSS files for my application instead of just one (which isn't a problem for this project but could easily be joined together in some other part of the pipeline). I avoided all uses of @import and processed all .css files in the extras directory. It's a little less flexible (can't simply edit a line to exclude a file) but doesn't impact my project. The benefit to this approach is that no changes to modular-css or custom PostCSS plugins are needed.

The old modular-css Svelte plugin still works like it used to, and I used the same namer function for the extra files so that the processed class names are predictable based on the relevant portion of the source file's path. For example, class .my-card in /src/library/components/card/card.css is transformed into the same class name as class .my-card in /extras/library/components/card/card.css. The benefit of this is approach is automated scoping of class names (the whole point of modular-css, right?) that can be modified for dev and prod purposes. For example, in dev, the class above is readable: .library-components-card-card_my-card and in prod it's compact: .abc123_my-card (where abc123 is a short hash of library-components-card-card).

No biggie, but I couldn't get the official modular-css plugin to behave, possibly due to version mismatches. But it was easy to use the JS API directly in a custom Rollup plugin of only 40 sparse lines of code.

tivac commented 2 years ago

I couldn't get the official modular-css plugin to behave

Would be interested in more context on what you mean here, but glad you've got a solution that works for your use-case.

ivanjonas commented 2 years ago

Haha I figured you would be. And I messed up, I actually used the glob package instead of the core JS API... It's been a few weeks...

When I ran modular-css extras/**/*.css on the CLI, the expected files were processed and included in the final output. Likewise, when I use the API thusly:

module.exports = () => ({
    name : "extra-styles-plugin",
    async buildStart() {
        const processor = await glob({
            search : [ "extras/**/*.css" ],
            namer,
        });

        const result = await processor.output();

        this.emitFile({
            type   : "asset",
            name   : EXTRA_STYLES_FILENAME,
            source : result.css,
        });
    },
});

But the Rollup plugin like this did not process any files. I played with passing in processor, rewrite, and styleExports, but in every case the produced metadata.json reported the same thing: no files were included.

modularCssRollup({
    include : "extras/**/*.css",
    namer,
})
ivanjonas commented 2 years ago

I can close this issue out, I suppose. I'm not at all going to benefit from getting that "message" reporting thing on @imported files. Up to you. Heh, even in that case, you would probably prefer to write a clean github issue for that and leave out all my junk.

tivac commented 2 years ago

The include option for @modular-css/rollup doesn't tell it to pull in arbitrary files, it just tells it which files in the rollup module graph it should look at.

A plugin that adds globbing support to rollup for something like import * from "./extras/**/*.css"; seems more like what you wanted, but glad you're sorted.