Repository: tivac/modular-css
Pull request #890 (2022-09-05 10:58:02)

Improve consistency of composition map
<!--- Provide a general summary of your changes in the Title above -->

## Description
<!--- Describe your changes in detail -->
Currently a technically the same composition of multiple classes from other file gives slightly different composition information depending on how it has been declared.

<details>
<summary>Multiple composes</summary>

```css
// composes.css
.class {
  composes: someDep from 'lib';
  composes: anotherDep from 'lib';
}
```
give
```json
"composes.css": {
  "composes": [
    "anotherDep",
    "someDep",
    "class",
  ],
```
</details>

<details>
<summary>Single compose</summary>

```css
// composes.css
.class {
  composes: someDep, anotherDep from 'lib';
}
```
gives 
```diff
"composes.css": {
  "composes": [
-  "anotherDep",
    "someDep",
+   "anotherDep",
    "class",
  ],
```
</details>


## 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. -->
For sake of consistency. It gives an idea of order in which classes have been declared and it can be used for post-processing.

## 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. -->
Unit tests have been updated.

## Types of changes
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [ ] Bug fix (non-breaking change which fixes an issue)
- [x] 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.
- [x] I have added tests to cover my changes.
- [x] All new and existing tests passed.


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

Latest commit: 5280319883cef8d8ee81fe72e83629b7bdee6eb3

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/master/docs/adding-a-changeset.md).

[Click here if you're a maintainer who wants to add a changeset to this PR](https://github.com/plesiecki/modular-css/new/consistent-composition-map?filename=.changeset/forty-crabs-breathe.md&value=---%0A%22%40modular-css%2Fprocessor%22%3A%20patch%0A---%0A%0AImprove%20consistency%20of%20composition%20map%0A)


netlify[bot]: ### <span aria-hidden="true">✅</span> Deploy Preview for *m-css* ready!


|  Name | Link |
|---------------------------------|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 5280319883cef8d8ee81fe72e83629b7bdee6eb3 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/sites/m-css/deploys/6315d63c3a887a0008b34e11 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-890--m-css.netlify.app](https://deploy-preview-890--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.eyJ1cmwiOiJodHRwczovL2RlcGxveS1wcmV2aWV3LTg5MC0tbS1jc3MubmV0bGlmeS5hcHAifQ.HIA_5v1kBrzrtCKYHWYt9rZOGxTysEDU9H_WCowBaks)<br /><br />_Use your smartphone camera to open QR code link._</details> |
---

_To edit notification comments on pull requests, go to your [Netlify site settings](https://app.netlify.com/sites/m-css/settings/deploys#deploy-notifications)._
codesee-maps[bot]: <details>
<summary><b>👇 <a href="https://app.codesee.io/r/reviews?pr=890&src=https%3A%2F%2Fgithub.com%2Ftivac%2Fmodular-css">Click on the image</a> for a new way to code review</b>
</summary>

  - Make big changes easier — review code in small groups of related files
  - Know where to start — see the whole change at a glance
  - Take a code tour — explore the change with an interactive tour
  - Make comments and review — all fully sync’ed with github

    [Try it now!](https://app.codesee.io/r/reviews?pr=890&src=https%3A%2F%2Fgithub.com%2Ftivac%2Fmodular-css)
</details>

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

#### Legend
![CodeSee Map Legend](https://app.codesee.io/Legend_v2.png)
codecov[bot]: # [Codecov](https://codecov.io/gh/tivac/modular-css/pull/890?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Pat+Cavit) Report
> Merging [#890](https://codecov.io/gh/tivac/modular-css/pull/890?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Pat+Cavit) (5280319) into [main](https://codecov.io/gh/tivac/modular-css/commit/94f5c8e1e1025d1704c2bb944b0613b9f28982c4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Pat+Cavit) (94f5c8e) will **not change** coverage.
> The diff coverage is `100.00%`.

```diff
@@           Coverage Diff           @@
##             main     #890   +/-   ##
=======================================
  Coverage   99.71%   99.71%           
=======================================
  Files          37       37           
  Lines        1417     1417           
  Branches      230      230           
=======================================
  Hits         1413     1413           
  Misses          4        4           
```


| [Impacted Files](https://codecov.io/gh/tivac/modular-css/pull/890?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://codecov.io/gh/tivac/modular-css/pull/890/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Pat+Cavit#diff-cGFja2FnZXMvcHJvY2Vzc29yL3Byb2Nlc3Nvci5qcw==) | `100.00% <100.00%> (ø)` | |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Pat+Cavit). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Pat+Cavit)

tivac: I've grabbed this PR down locally and I'm doing some digging to see if I can understand where the unexpected ordering of these is coming from. I agree that it should be consistent whether you compose in a single decl or multiple, so I really appreciate the work to nail that down!
tivac: Ok, I see what was happening. Long and rambly explanation:

Before fully processing a file we scan it for any external file references and use those to start populating the internal dependency graph and figuring out which files we'll need to walk. Due to some code added in [the postcss@8 transition ](https://github.com/tivac/modular-css/pull/795) *every single time we added dependency info* we dumped all the info that was there already. This wasn't a problem for the final output, because the more-detailed second pass would pick it up. It did, however, lead to an oddity in the composition output you noticed. Whichever `composes` declaration came last would be the only one remaining in the dep graph until those later passes. leading to the composition info being out of order. When we output classes we re-walk the dependency graph so those are fine fortunately 😅

So what I've done is extended the concept of validity that the processor already had for files down to selector graph nodes as well. We only dump existing selector dependencies out of the graph if the selector itself is invalid now, and that seems to handle all the existing tests **and** fix up the inconsistent output you were seeing.

This one was so weird, thank you again for noticing and investigating @plesiecki!
plesiecki: Lovely, great work @tivac. Looking forward to the release!

Patch:
diff --git a/.changeset/giant-garlics-worry.md b/.changeset/giant-garlics-worry.md
new file mode 100644
index 00000000..e3a97fe6
--- /dev/null
+++ b/.changeset/giant-garlics-worry.md
@@ -0,0 +1,5 @@
+---
+"@modular-css/processor": patch
+---
+
+Improve consistency of composition map by ensuring that both forms of `composes` declaration correctly order the classes.
diff --git a/packages/processor/processor.js b/packages/processor/processor.js
index 50827e65..7be0d21b 100644
--- a/packages/processor/processor.js
+++ b/packages/processor/processor.js
@@ -33,6 +33,7 @@ const {
     filterByPrefix,
 
     FILE_PREFIX,
+    SELECTOR_PREFIX,
 } = keys;
 
 let fs;
@@ -229,13 +230,23 @@ class Processor {
             throw new Error(`Unknown file: ${normalized}`);
         }
 
+        // Mark the file & all files that depend on it invalid
         [ ...filterByPrefix(FILE_PREFIX, this._graph.dependantsOf(key)), normalized ].forEach((file) => {
-            this._log("invalidate()", file);
-
             this._files[file].valid = false;
 
             this._ids.delete(file.toLowerCase());
         });
+
+        // Mark all selectors in the file invalid
+        filterByPrefix(SELECTOR_PREFIX, this._graph.dependenciesOf(key), { clean : false }).forEach((sKey) => {
+            const data = this._graph.getNodeData(sKey);
+
+            if(data.file !== normalized) {
+                return;
+            }
+
+            data.valid = false;
+        });
     }
 
     // Get the dependency order for a file or the entire tree
@@ -418,10 +429,13 @@ class Processor {
             this._graph.addNode(sKey, {
                 file,
                 selector,
+                valid : true,
             });
 
             this._graph.getNodeData(fKey).selectors.push(sKey);
             this._graph.addDependency(fKey, sKey);
+        } else {
+            this._graph.getNodeData(sKey).valid = true;
         }
 
         return sKey;
@@ -484,8 +498,8 @@ class Processor {
         // Add selector and its dependencies to the graph
         const selectorId = selectorKey(name, selector);
 
-        // Remove any existing dependencies for the selector
-        if(graph.hasNode(selectorId)) {
+        // 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);
             });


Test patch:
diff --git a/packages/processor/test/__snapshots__/api.test.js.snap b/packages/processor/test/__snapshots__/api.test.js.snap
index ff9c5656..fe720d3a 100644
--- a/packages/processor/test/__snapshots__/api.test.js.snap
+++ b/packages/processor/test/__snapshots__/api.test.js.snap
@@ -231,6 +231,7 @@ exports[`/processor.js API .output() should order output by dependencies, then a
 .mc_folder { margin: 2px; }
 /* packages/processor/test/specimens/folder/folder2.css */
 .mc_folder2 { color: green; }
+.mc_foo { display: block; }
 /* packages/processor/test/specimens/folder/subfolder/subfolder.css */
 .mc_subfolder { color: yellow; }
 /* packages/processor/test/specimens/composes.css */
diff --git a/packages/processor/test/__snapshots__/composition.test.js.snap b/packages/processor/test/__snapshots__/composition.test.js.snap
index 0eb67c4d..71070751 100644
--- a/packages/processor/test/__snapshots__/composition.test.js.snap
+++ b/packages/processor/test/__snapshots__/composition.test.js.snap
@@ -71,11 +71,32 @@ Object {
 }
 `;
 
-exports[`/processor.js composition should compose from other files 1`] = `
+exports[`/processor.js composition should compose from other files in multiple declarations 1`] = `
+Object {
+  "packages/processor/test/specimens/composes2.css": Object {
+    "composes": Array [
+      "mc_folder2",
+      "mc_foo",
+      "mc_composes",
+    ],
+  },
+  "packages/processor/test/specimens/folder/folder2.css": Object {
+    "folder2": Array [
+      "mc_folder2",
+    ],
+    "foo": Array [
+      "mc_foo",
+    ],
+  },
+}
+`;
+
+exports[`/processor.js composition should compose from other files in single declaration 1`] = `
 Object {
   "packages/processor/test/specimens/composes.css": Object {
     "composes": Array [
       "mc_folder2",
+      "mc_foo",
       "mc_composes",
     ],
   },
@@ -83,6 +104,9 @@ Object {
     "folder2": Array [
       "mc_folder2",
     ],
+    "foo": Array [
+      "mc_foo",
+    ],
   },
 }
 `;
diff --git a/packages/processor/test/composition.test.js b/packages/processor/test/composition.test.js
index 9bc5c506..12392e1b 100644
--- a/packages/processor/test/composition.test.js
+++ b/packages/processor/test/composition.test.js
@@ -182,7 +182,7 @@ describe("/processor.js", () => {
             expect(compositions).toMatchSnapshot();
         });
 
-        it("should compose from other files", async () => {
+        it("should compose from other files in single declaration", async () => {
             await processor.file(require.resolve("./specimens/composes.css"));
 
             const { compositions } = await processor.output();
@@ -190,6 +190,14 @@ describe("/processor.js", () => {
             expect(compositions).toMatchSnapshot();
         });
 
+        it("should compose from other files in multiple declarations", async () => {
+            await processor.file(require.resolve("./specimens/composes2.css"));
+
+            const { compositions } = await processor.output();
+
+            expect(compositions).toMatchSnapshot();
+        });
+
         it("should compose with escaped classes", async () => {
             await processor.string(
                 "./escaped-classes.css",
diff --git a/packages/processor/test/specimens/composes.css b/packages/processor/test/specimens/composes.css
index 6d8a2a31..a21cf596 100644
--- a/packages/processor/test/specimens/composes.css
+++ b/packages/processor/test/specimens/composes.css
@@ -1,3 +1,3 @@
 .composes {
-    composes: folder2 from "./folder/folder2.css";
+    composes: folder2, foo from "./folder/folder2.css";
 }
diff --git a/packages/processor/test/specimens/composes2.css b/packages/processor/test/specimens/composes2.css
new file mode 100644
index 00000000..64fef22f
--- /dev/null
+++ b/packages/processor/test/specimens/composes2.css
@@ -0,0 +1,4 @@
+.composes {
+    composes: folder2 from "./folder/folder2.css";
+    composes: foo from "./folder/folder2.css";
+}
diff --git a/packages/processor/test/specimens/folder/folder2.css b/packages/processor/test/specimens/folder/folder2.css
index 417cff38..294fb42c 100644
--- a/packages/processor/test/specimens/folder/folder2.css
+++ b/packages/processor/test/specimens/folder/folder2.css
@@ -1,1 +1,2 @@
 .folder2 { color: green; }
+.foo { display: block; }
\ No newline at end of file