Repository: tivac/modular-css
Pull request #972 (2023-09-25 06:48:48)

Vite updates & invalidation fixes
<!--- Provide a general summary of your changes in the Title above -->

## Description
<!--- Describe your changes in detail -->
Working to make vite reloading more reliable, especially around dependencies. Also fixing an issue that could leave stale compositions around when re-running the processor against invalidated files.

## Motivation and Context
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
vite reload behavior has been extremely annoying at my day job.

## How Has This Been Tested?
<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, and the tests you ran to -->
<!--- see how your change affects other areas of the code, etc. -->
Manual testing _only_ so far.

## Types of changes
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)

## Checklist:
<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->
- [x] My code follows the code style of this project.
- [ ] My change requires a change to the documentation.
- [ ] I have updated the documentation accordingly.
- [x] I have read the **CONTRIBUTING** document.
- [ ] I have added tests to cover my changes.
- [ ] All new and existing tests passed.
- [ ] I have added a changeset for my change.


changeset-bot[bot]: ###  ⚠️  No Changeset found

Latest commit: 35bda9c6c2b4c0f235721423b70cc02a87ec432a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. **If these changes should result in a version bump, you need to add a changeset.**

<details><summary>This PR includes no changesets</summary>

  When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

</details>

[Click here to learn what changesets are, and how to add one](https://github.com/changesets/changesets/blob/main/docs/adding-a-changeset.md).

[Click here if you're a maintainer who wants to add a changeset to this PR](https://github.com/tivac/modular-css/new/vite-updates?filename=.changeset/itchy-monkeys-protect.md&value=---%0A%22%40modular-css%2Fprocessor%22%3A%20patch%0A%22%40modular-css%2Fvite%22%3A%20patch%0A---%0A%0AVite%20updates%20%26%20invalidation%20fixes%0A)


codesee-maps[bot]: <details open>
<summary><b>👇 <a href="https://app.codesee.io/r/reviews?pr=972&src=https%3A%2F%2Fgithub.com%2Ftivac%2Fmodular-css">Click on the image</a> for a new way to code review</b>
</summary>

####

[![Review these changes using an interactive CodeSee Map](https://s3.us-east-2.amazonaws.com/maps.codesee.io/images/github/tivac/modular-css/972/ea0da5fe/35bda9c6c2b4c0f235721423b70cc02a87ec432a.svg)](https://app.codesee.io/r/reviews?pr=972&src=https%3A%2F%2Fgithub.com%2Ftivac%2Fmodular-css)

#### Legend
<img src="https://app.codesee.io/Legend_v3.png" alt="CodeSee Map legend" width="220" height="156">
</details>
netlify[bot]: ### <span aria-hidden="true">✅</span> Deploy Preview for *m-css* ready!


|  Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 35bda9c6c2b4c0f235721423b70cc02a87ec432a |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/sites/m-css/deploys/65112d51dfd798000844c923 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-972--m-css.netlify.app](https://deploy-preview-972--m-css.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br />![QR Code](https://app.netlify.com/qr-code/eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJ1cmwiOiJodHRwczovL2RlcGxveS1wcmV2aWV3LTk3Mi0tbS1jc3MubmV0bGlmeS5hcHAifQ.AuxzxNBTdT8K7gUf0Wjad-f2KQ7RpRIYwOlGuOIkyss)<br /><br />_Use your smartphone camera to open QR code link._</details> |
---

_To edit notification comments on pull requests, go to your [Netlify site configuration](https://app.netlify.com/sites/m-css/configuration/deploys#deploy-notifications)._
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/tivac/modular-css/pull/972?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Pat+Cavit) Report
Patch coverage: **`100.00%`** and project coverage change: **`-0.07%`** :warning:
> Comparison is base [(`4fa842e`)](https://app.codecov.io/gh/tivac/modular-css/commit/4fa842e2c087b5b99970c8e67e76398f29283f2f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Pat+Cavit) 99.72% compared to head [(`35bda9c`)](https://app.codecov.io/gh/tivac/modular-css/pull/972?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Pat+Cavit) 99.65%.

> :exclamation: Current head 35bda9c differs from pull request most recent head d9dc358. Consider uploading reports for the commit d9dc358 to get more accurate results

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main     #972      +/-   ##
==========================================
- Coverage   99.72%   99.65%   -0.07%     
==========================================
  Files          37       37              
  Lines        1441     1449       +8     
  Branches      237      239       +2     
==========================================
+ Hits         1437     1444       +7     
- Misses          4        5       +1     
```


| [Files Changed](https://app.codecov.io/gh/tivac/modular-css/pull/972?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Pat+Cavit) | Coverage Δ | |
|---|---|---|
| [packages/processor/processor.js](https://app.codecov.io/gh/tivac/modular-css/pull/972?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Pat+Cavit#diff-cGFja2FnZXMvcHJvY2Vzc29yL3Byb2Nlc3Nvci5qcw==) | `99.61% <100.00%> (-0.39%)` | :arrow_down: |


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/tivac/modular-css/pull/972?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Pat+Cavit).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Pat+Cavit).


Patch:
diff --git a/.changeset/angry-shoes-double.md b/.changeset/angry-shoes-double.md
new file mode 100644
index 00000000..a11334a0
--- /dev/null
+++ b/.changeset/angry-shoes-double.md
@@ -0,0 +1,7 @@
+---
+"@modular-css/processor": patch
+"@modular-css/vite": patch
+---
+
+Improved vite integration, now correctly invalidating files when a file is changed or deleted.
+Improved invalidation in the processor, preventing stale `@value` or `composes` references from being output.
diff --git a/packages/processor/processor.js b/packages/processor/processor.js
index 70d8ae83..6b24da96 100644
--- a/packages/processor/processor.js
+++ b/packages/processor/processor.js
@@ -33,7 +33,6 @@ const {
     filterByPrefix,
 
     FILE_PREFIX,
-    SELECTOR_PREFIX,
 } = keys;
 
 let fs;
@@ -238,8 +237,8 @@ class Processor {
         });
 
         // Mark all selectors in the file invalid
-        filterByPrefix(SELECTOR_PREFIX, this._graph.dependenciesOf(key), { clean : false }).forEach((sKey) => {
-            const data = this._graph.getNodeData(sKey);
+        this._graph.dependenciesOf(key).forEach((dep) => {
+            const data = this._graph.getNodeData(dep);
 
             if(data.file !== normalized) {
                 return;
@@ -247,6 +246,8 @@ class Processor {
 
             data.valid = false;
         });
+
+        this._log("invalidate()", normalized);
     }
 
     // Get the dependency order for a file or the entire tree
@@ -262,9 +263,23 @@ class Processor {
             throw new Error(`Unknown file: ${normalized}`);
         }
 
-        const dependencies = this._graph.dependenciesOf(key);
+        return filterByPrefix(FILE_PREFIX, this._graph.dependenciesOf(key));
+    }
+
+    // Get the file dependents for a specific file
+    fileDependents(file) {
+        if(!file) {
+            throw new Error("fileDepenendents() must be called with a file");
+        }
+
+        const normalized = this._normalize(file);
+        const key = fileKey(normalized);
+
+        if(!this._graph.hasNode(key)) {
+            throw new Error(`Unknown file: ${normalized}`);
+        }
 
-        return filterByPrefix(FILE_PREFIX, dependencies);
+        return filterByPrefix(FILE_PREFIX, this._graph.dependantsOf(key));
     }
 
     // Get the ultimate output for specific files or the entire tree
@@ -496,15 +511,6 @@ class Processor {
         }
 
         // Add selector and its dependencies to the graph
-        const selectorId = selectorKey(name, selector);
-
-        // Remove any existing dependencies for the selector if it is invalid
-        if(graph.hasNode(selectorId) && !graph.getNodeData(selectorId).valid) {
-            graph.dependenciesOf(selectorId).forEach((other) => {
-                graph.removeDependency(selectorId, other);
-            });
-        }
-
         this._addSelector(name, selector);
 
         refs.forEach(({ name : depSelector }) => {
@@ -597,7 +603,23 @@ class Processor {
             return;
         }
 
-        const fKey = this._addFile(name);
+        const fKey = fileKey(name);
+
+        // Clean up old graph dependencies for this node since it's about to be parsed
+        // and they'll all be recreated anyways
+        if(graph.hasNode(fKey)) {
+            graph.directDependenciesOf(fKey).forEach((dep) => {
+                const data = this._graph.getNodeData(dep);
+
+                if(data.file !== name) {
+                    return;
+                }
+                
+                graph.directDependenciesOf(dep).forEach((dep2) => graph.removeDependency(dep, dep2));
+            });
+        }
+
+        this._addFile(name);
 
         this._log("_before()", name);
 
diff --git a/packages/vite/vite.js b/packages/vite/vite.js
index 0a22b232..2f68d224 100644
--- a/packages/vite/vite.js
+++ b/packages/vite/vite.js
@@ -59,11 +59,26 @@ module.exports = (
     }
 
     let viteServer;
+    let viteConfig;
+
+    const watchFile = (file) => {
+        const slashed = slash(file);
+
+        if(viteServer) {
+            viteServer.watcher.add(slashed);
+        } else if(viteConfig.build.watch) {
+            this.addWatchFile(slashed);
+        }
+    };
 
     return {
         name    : "@modular-css/vite",
         enforce : "pre",
 
+        configResolved(config) {
+            viteConfig = config;
+        },
+
         configureServer(instance) {
             viteServer = instance;
 
@@ -75,6 +90,27 @@ module.exports = (
                 }
 
                 processor.invalidate(file);
+
+                // Make sure vite knows that files that depend on us might've changed too
+                processor.fileDependents(file).forEach((dep) => {
+                    viteServer.watcher.emit("change", dep);
+                });
+            });
+
+            viteServer.watcher.on("unlink", (file) => {
+                if(!processor.has(file)) {
+                    return;
+                }
+
+                // Have to grab deps _before_ removal
+                const deps = processor.fileDependents(file);
+                
+                processor.remove(file);
+                
+                // Invalidate dependencies of the file that was removed
+                deps.forEach((dep) => {
+                    viteServer.watcher.emit("change", dep);
+                });
             });
         },
 
@@ -82,7 +118,7 @@ module.exports = (
             log("build start");
 
             // Watch any files already in the procesor
-            Object.keys(processor.files).forEach((file) => this.addWatchFile(file));
+            Object.keys(processor.files).forEach((file) => watchFile(file));
         },
 
         async resolveId(source) {
@@ -191,7 +227,7 @@ module.exports = (
 
             const deps = processor.fileDependencies(file);
 
-            deps.forEach((dep) => this.addWatchFile(slash(dep)));
+            deps.forEach((dep) => watchFile(dep));
 
             // TODO: Gets CSS order right by forcing them to load their dependent CSS first.
             // Feels very brittle and overkill, but this is what we've got for now


Test patch:
diff --git a/packages/processor/test/__snapshots__/api.test.js.snap b/packages/processor/test/__snapshots__/api.test.js.snap
index bf5c9470..ae8da0c0 100644
--- a/packages/processor/test/__snapshots__/api.test.js.snap
+++ b/packages/processor/test/__snapshots__/api.test.js.snap
@@ -80,6 +80,12 @@ exports[`/processor.js API .fileDependencies() should return the overall order o
 ]
 `;
 
+exports[`/processor.js API .fileDependents() should return the dependencies of the specified file 1`] = `
+[
+  "packages/processor/test/specimens/start.css",
+]
+`;
+
 exports[`/processor.js API .invalidate() should invalidate a relative file 1`] = `
 [
   [
diff --git a/packages/processor/test/api.test.js b/packages/processor/test/api.test.js
index 078cd2d5..a02e316b 100644
--- a/packages/processor/test/api.test.js
+++ b/packages/processor/test/api.test.js
@@ -241,6 +241,33 @@ describe("/processor.js", () => {
             });
         });
 
+        describe(".fileDependents()", () => {
+            it("should return the dependencies of the specified file", async () => {
+                await processor.file("./packages/processor/test/specimens/start.css");
+
+                expect(
+                    relative(processor.fileDependents(require.resolve("./specimens/local.css")))
+                )
+                .toMatchSnapshot();
+            });
+
+            it("should throw if no file is specified", async () => {
+                await processor.file("./packages/processor/test/specimens/start.css");
+
+                expect(() => processor.fileDependents()).toThrow("must be called with a file");
+            });
+
+            it("should throw on requesting an invalid file", async () => {
+                await processor.string("./does/not/exist.css", dedent(`
+                    .foo {
+                        color: red;
+                    }
+                `));
+
+                expect(() => processor.fileDependents("./also/does/not/exist.css")).toThrow("Unknown file: ");
+            });
+        });
+
         describe(".output()", () => {
             it("should reject unknown files", async () => {
                 await expect(processor.output({