Repository: tivac/modular-css
Pull request #898 (2022-10-07 09:15:13)

[css-to-js] Invalid JS when mixing rules with composes
<!--- Provide a general summary of your changes in the Title above -->

## Description
<!--- Describe your changes in detail -->
Hello 👋 I've spotted an issue with `css-to-js` module. [For CSS](https://github.com/plesiecki/modular-css/blob/df11a5be29fc66516de7ae60836a9adf515f61cd/packages/css-to-js/test/specimens/composes/custom-between-rules.css#L1-L9):
```css
.rule { 
  display: flex;
}
.custom {
  color: pink;
}
.rule {
  composes: custom;
}
```
it produces an [invalid JS](https://github.com/plesiecki/modular-css/blob/df11a5be29fc66516de7ae60836a9adf515f61cd/packages/css-to-js/test/__snapshots__/api.test.js.snap#L61) which looks like this:
```js
const rule =  + " " + "mc9c938a76_rule"
```

## Motivation and Context
A valid CSS can give an invalid JS.

## How Has This Been Tested?
Unit tests have been updated.

**For the second test a snapshot have been updated manually so the test should fail.**

## 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! -->
- [ ] My code follows the code style of this project.
- [ ] My change requires a change to the documentation.
- [ ] I have updated the documentation accordingly.
- [ ] I have read the **CONTRIBUTING** document.
- [ ] I have added tests to cover my changes.
- [ ] All new and existing tests passed.


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

Latest commit: 08084ead7dcf5e21fc9afef136424fcb7e84715f

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/css-to-js-missing-var-syntax-err?filename=.changeset/dry-poems-help.md&value=---%0A%22%40modular-css%2Fcss-to-js%22%3A%20patch%0A---%0A%0A%5Bcss-to-js%5D%20Invalid%20JS%20when%20mixing%20rules%20with%20composes%0A)


codesee-maps[bot]: <details>
<summary><b>👇 <a href="https://app.codesee.io/r/reviews?pr=898&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=898&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/898/d11c60bf/08084ead7dcf5e21fc9afef136424fcb7e84715f.svg)](https://app.codesee.io/r/reviews?pr=898&src=https%3A%2F%2Fgithub.com%2Ftivac%2Fmodular-css)

#### Legend
![CodeSee Map Legend](https://app.codesee.io/Legend_v2.png)
netlify[bot]: ### <span aria-hidden="true">✅</span> Deploy Preview for *m-css* ready!


|  Name | Link |
|---------------------------------|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 08084ead7dcf5e21fc9afef136424fcb7e84715f |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/sites/m-css/deploys/633fee221d6aa70009046f28 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-898--m-css.netlify.app](https://deploy-preview-898--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.eyJ1cmwiOiJodHRwczovL2RlcGxveS1wcmV2aWV3LTg5OC0tbS1jc3MubmV0bGlmeS5hcHAifQ.jTxfh-MDqoaKX_EXH_dEemEi6mZxAD_AnU5BMSblZMM)<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)._
plesiecki: It works for 81fd1bbac239c69a9adaa0f3dcc2874df6090b68, @tivac what do you think about that change?
codecov[bot]: # [Codecov](https://codecov.io/gh/tivac/modular-css/pull/898?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Pat+Cavit) Report
Base: **99.71**% // Head: **99.71**% // Increases project coverage by **`+0.00%`** :tada:
> Coverage data is based on head [(`81fd1bb`)](https://codecov.io/gh/tivac/modular-css/pull/898?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Pat+Cavit) compared to base [(`e64362e`)](https://codecov.io/gh/tivac/modular-css/commit/e64362e087f4ae6113a1b622e59dab827a9f2e97?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Pat+Cavit).
> Patch coverage: 100.00% of modified lines in pull request are covered.

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


```diff
@@           Coverage Diff           @@
##             main     #898   +/-   ##
=======================================
  Coverage   99.71%   99.71%           
=======================================
  Files          37       37           
  Lines        1423     1425    +2     
  Branches      232      232           
=======================================
+ Hits         1419     1421    +2     
  Misses          4        4           
```


| [Impacted Files](https://codecov.io/gh/tivac/modular-css/pull/898?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Pat+Cavit) | Coverage Δ | |
|---|---|---|
| [packages/css-to-js/css-to-js.js](https://codecov.io/gh/tivac/modular-css/pull/898/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Pat+Cavit#diff-cGFja2FnZXMvY3NzLXRvLWpzL2Nzcy10by1qcy5qcw==) | `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)

</details>

[:umbrella: View full report at Codecov](https://codecov.io/gh/tivac/modular-css/pull/898?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Pat+Cavit).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](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).

plesiecki: > So the issue being fixed here is that when we walked the exported keys sometimes we were walking classes out of dependency order, and your fix is to always walk all the local classes first and ensure they've had a JS identifier created before actually creating the value for them?

Basically yes.



Patch:
diff --git a/.changeset/warm-dryers-thank.md b/.changeset/warm-dryers-thank.md
new file mode 100644
index 00000000..219d258a
--- /dev/null
+++ b/.changeset/warm-dryers-thank.md
@@ -0,0 +1,5 @@
+---
+"@modular-css/css-to-js": patch
+---
+
+Class ordering in `.css` files could lead to invalid identifiers being created, so always make sure every class defined in the `.css` file has a local JS identifier before building the string concatenation to represent composition.
diff --git a/packages/css-to-js/css-to-js.js b/packages/css-to-js/css-to-js.js
index f67dccec..0dfba4da 100644
--- a/packages/css-to-js/css-to-js.js
+++ b/packages/css-to-js/css-to-js.js
@@ -227,13 +227,18 @@ exports.transform = (file, processor, opts = {}) => {
         namedExports.push(esm(unique, DEFAULT_VALUES));
     }
 
-    // Create vars representing exported classes & use them in local var definitions
+    // Collect local exports first
     exportedKeys.forEach((key) => {
-        const elements = [];
         const unique = deconflict(identifiers, key);
-        const sKey = selectorKey(id, key);
 
         internalsMap.set(key, unique);
+    });
+
+    // Create vars representing exported classes & use them in local var definitions
+    exportedKeys.forEach((key) => {
+        const elements = [];
+        const sKey = selectorKey(id, key);
+        const unique = internalsMap.get(key);
 
         // Build the list of composed classes for this class
         if(graph.hasNode(sKey)) {


Test patch:
diff --git a/packages/css-to-js/test/__snapshots__/api.test.js.snap b/packages/css-to-js/test/__snapshots__/api.test.js.snap
index efe33749..8626e746 100644
--- a/packages/css-to-js/test/__snapshots__/api.test.js.snap
+++ b/packages/css-to-js/test/__snapshots__/api.test.js.snap
@@ -43,6 +43,34 @@ Array [
 ]
 `;
 
+exports[`@modular-css/css-to-js API should generate javascript from composes: code 1`] = `
+const custom = "mca72bf2bd_custom"
+const rule = custom + " " + "mca72bf2bd_rule"
+export default {
+    custom,
+rule
+};
+
+export {
+    custom,
+rule
+};
+`;
+
+exports[`@modular-css/css-to-js API should generate javascript from multiple composes: code 1`] = `
+const rule = custom + " " + "mc9c938a76_rule"
+const custom = "mc9c938a76_custom"
+export default {
+    rule,
+custom
+};
+
+export {
+    rule,
+custom
+};
+`;
+
 exports[`@modular-css/css-to-js API should generate javascript: code 1`] = `
 const a = "mc74f3fa7b_a"
 export default {
diff --git a/packages/css-to-js/test/api.test.js b/packages/css-to-js/test/api.test.js
index 91e55712..cce82971 100644
--- a/packages/css-to-js/test/api.test.js
+++ b/packages/css-to-js/test/api.test.js
@@ -20,10 +20,7 @@ expect.addSnapshotSerializer({
 });
 
 const resolvers = [
-    (src, file) =>
-        // console.log({ src, file, result : path.join(path.dirname(src), file) });
-        
-         path.join(path.dirname(src), file),
+    (src, file) => path.join(path.dirname(src), file),
 ];
 
 describe("@modular-css/css-to-js API", () => {
@@ -168,6 +165,27 @@ describe("@modular-css/css-to-js API", () => {
         expect(namedExports).toEqual([ "$values", "b" ]);
     });
 
+
+    it("should generate javascript from composes", async () => {
+        const processor = new Processor({ resolvers });
+
+        await processor.file(require.resolve("./specimens/composes/custom-first-rule.css"));
+
+        const { code } = transform(require.resolve("./specimens/composes/custom-first-rule.css"), processor);
+
+        expect(code).toMatchSnapshot("code");
+    });
+
+    it("should generate javascript from multiple composes", async () => {
+        const processor = new Processor({ resolvers });
+
+        await processor.file(require.resolve("./specimens/composes/custom-between-rules.css"));
+
+        const { code } = transform(require.resolve("./specimens/composes/custom-between-rules.css"), processor);
+
+        expect(code).toMatchSnapshot("code");
+    });
+
     it.each([
         true,
         false,
diff --git a/packages/css-to-js/test/specimens/composes/custom-between-rules.css b/packages/css-to-js/test/specimens/composes/custom-between-rules.css
new file mode 100644
index 00000000..6ff3bc1e
--- /dev/null
+++ b/packages/css-to-js/test/specimens/composes/custom-between-rules.css
@@ -0,0 +1,9 @@
+.rule { 
+  display: flex;
+}
+.custom {
+  color: pink;
+}
+.rule {
+  composes: custom;
+}
\ No newline at end of file
diff --git a/packages/css-to-js/test/specimens/composes/custom-first-rule.css b/packages/css-to-js/test/specimens/composes/custom-first-rule.css
new file mode 100644
index 00000000..06d4b861
--- /dev/null
+++ b/packages/css-to-js/test/specimens/composes/custom-first-rule.css
@@ -0,0 +1,9 @@
+.custom {
+  color: pink;
+}
+.rule { 
+  display: flex;
+}
+.rule {
+  composes: custom;
+}
\ No newline at end of file