fix: handling of css module imports

[GeorgeTaveras1231]

Problem

This tool does not properly handle errors, that is being addressed in #217. Once that fix is applied, some errors that may be occurring for many developers will begin to surface, one of which is addressed by this PR.

The error that this fixes is a compile error caused by the fact that postcss-modules only parses valid CSS. So in the scenario where a developer uses the CSS Modules syntax to compose or import a value (via @value) from an SCSS file, postcss-modules will hit a syntax error if it encounters any SASS specific syntax.

Solution

This solution stubs any CSS Module that is imported from the file being compiled.

References

The original discovery of this issue was called out here but the PR had other changes that made it hard to follow:

[skovy]

I'm struggling to understand why this is necessary? Can you provide some more details and examples (particularly, test cases)?

Right now, I'm hesitant to merge this and seems like there's maybe a configuration issue or something and the tool should already solve this?

[GeorgeTaveras1231]

Let me try to explain this as thoroughly as possible:

Lets say you have this SCSS file

my-scss-file.scss

.my-class {
  composes: some-other-class from './some-other.scss'
}

Here is the compilation process (at a high-level) where I called out with exclamation marks where the issue occurs. (Look for !!!SASS CODE!!!. In that part of the process, SASS code is given to postcss-modules. postcss-modules can only handle plain CSS, so it will raise an error. (See bottom of comment for the solution and how it fixes this)

flowchart TD;
  typed-scss-modules{{typed-scss-modules}};
  sass{{sass}}
  postcss-modules{{postcss-modules}};
  parseSass{parse SASS}
  extractTypesFromSCSS{extract types from SCSS}
  compileCSSModules{compile CSS Modules};
  parseCSSModules{parse CSS Modules};
  findCSSModuleImports{find CSS Module imports};

  typed-scss-modules -- my-scss-file.scss -->extractTypesFromSCSS;
  extractTypesFromSCSS -- my-scss-file.scss --> sass;
  sass -- my-scss-file.scss --> parseSass;
  parseSass -- Plain CSS for my-scss-file.scss --> compileCSSModules;
  compileCSSModules -- Plain CSS for my-scss-file.scss --> postcss-modules;
  postcss-modules -- Plain CSS for my-scss-file.scss --> parseCSSModules;
  parseCSSModules -- Plain CSS for my-scss-files.scss --> findCSSModuleImports;
  findCSSModuleImports -- !!!SASS CODE!!! some-other.scss --> postcss-modules;
  linkStyle 7 stroke:red;

Loading

The solution in this PR is to stub CSS modules that are imported via composes or @value, changing the flow to

flowchart TD;
  typed-scss-modules{{typed-scss-modules}};
  sass{{sass}}
  postcss-modules{{postcss-modules}};
  parseSass{parse SASS}
  extractTypesFromSCSS{extract types from SCSS}
  compileCSSModules{compile CSS Modules};
  parseCSSModules{parse CSS Modules};
  findCSSModuleImports{find CSS Module imports};

  typed-scss-modules -- my-scss-file.scss -->extractTypesFromSCSS;
  extractTypesFromSCSS -- my-scss-file.scss --> sass;
  sass -- my-scss-file.scss --> parseSass;
  parseSass -- Plain CSS for my-scss-file.scss --> compileCSSModules;
  compileCSSModules -- Plain CSS for my-scss-file.scss --> postcss-modules;
  postcss-modules -- Plain CSS for my-scss-file.scss --> parseCSSModules;
  parseCSSModules -- Plain CSS for my-scss-files.scss --> findCSSModuleImports;
  findCSSModuleImports -- stub for some-other.scss --> postcss-modules;
  linkStyle 7 stroke:green;

Loading

And to address a comment you made in the previous PR where you asked if I could use a custom (sass) importer to solve the problem. The answer to that is no because the error doesn't happen during the SASS resolve/build process. SASS importers would work if the problem surfaced when using@import or @use. The problem surfaces when using CSS Module syntax that is parsed by postcss-modules (@value and composes).

---

I will follow up with test cases soon

[GeorgeTaveras1231]

Note that the diagrams above are mermaid diagrams that seem to only render in Github web. In case anyone opens this in a mobile app and is confused by the code snippet 😅

[skovy]

Interesting. I need to think on the diagrams a bit more to fully wrap my head around.

Since this error example code is an example included in the postcss-modules readme (https://www.npmjs.com/package/postcss-modules) I'm wondering is there something we could do to enable postcss-modules to handle this? I'm wondering if we can fix the root cause of these errors instead of patching them

[GeorgeTaveras1231]

@skovy That's a valid position to take, and in fact, it was the original position I took when first tackling this problem. However, now I think that this approach is much better because 1. The resulting types will ultimately be the same. And 2. Properly handling css module imports will introduce the need for more SASS configurations, and would be much worse if there is a non-sass CSS Module import -- for example, if a project is mid-migration from some other CSS pre-processor to SASS. Using the commonly used bundlers, its perfectly reasonable that via compose or @value a CSS Module file will import Less or Stylus code.

The drawback with the stubbing approach that I took is that the CSS Module code is not ran in a way the resembles how it is intended to be used. But I think this drawback is suitable for this tool because you should not need this tool to validate the correctness of your code (at least not entirely). As long as it is able to generate the ts files, and surface errors scoped to the config and the individual SASS files, that would be enough to provide the value promised by this tool.

The full correctness of the CSS Module tree should be validated by the developer's bundler.

[GeorgeTaveras1231]

@skovy I reproduced the error and pushed this up so you can play around with it if you would like.

I pushed it as a PR so you can easily inspect the code changes I made to reproduce the error in the original code.

The changes just include the addition of a composes: directive in a sass file to a file that uses syntax only parseable by SASS

To see the results I ran npm test -- __tests__/cli.test.ts

Expected: test to pass
Actual: Produces the following error

Command failed: npm run typed-scss-modules "examples/basic/**/*.scss" -- --includePaths examples/basic/core --aliases.~alias variables --banner '// example banner'
    /<rootPath>/typed-scss-modules/node_modules/postcss/lib/input.js:106
          result = new CssSyntaxError(
                   ^
    CssSyntaxError: /<rootPath>/typed-scss-modules/examples/basic/feature-a/another-style.scss:3:19: Unknown word
    You tried to parse SCSS with the standard CSS parser; try again with the postcss-scss parser

Note: The error is raised even without the addition of the code in #217. This is good and suggests that the issue I mentioned where errors are swallowed is not as bad as I thought -- I think #217 is still an improvement to the error handling since it raises issues in the config file, and uses stderr for warnings.

[skovy]

I'm wondering if we make postcss capable of handling these imports with https://github.com/postcss/postcss-scss and remove node-sass and dart-sass which could eliminate the challenge of needing those 🤔

[GeorgeTaveras1231]

@skovy last I checked, the postcss-modules library uses its own postcss parse process for the imports, and it doesn't allow customizing the parser 😬. You can look into it, but I doubt that has changed.

Also, I'm pretty sure you can't replace node-sass/dart-sass.

postcss-scss only gives you the ability to parse SCSS AST. This does not include the SASS runtime. If you didn't use dar-sass/node-sass you wouldn't be able to know what classes code like the following produces - so you won't be able to generate the types.

@mixin create-class ($suffix) {
  .my-dynamic-class-#{$suffix} {
    // ommitted
  }
}

@include create-class(suffix-1);
@include create-class(suffix-2);
@include create-class(suffix-3);

[GeorgeTaveras1231]

As an aside, I'm going to begin stepping back from this project as it has been taking quite long to merge these PRs and a lot of energy explaining how these things work.

In my opinion, I think the solution is simple, reliable, and does not have unintended side effects. Feel free to merge it whenever you feel comfortable, close it if you feel strongly in opposition to it, or update it if you find anything worth changing.

[GeorgeTaveras1231]

@skovy Checking if you've had a chance to evaluate this change.