Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(config): support modules config to customize css modules processing #218

Conversation

GeorgeTaveras1231
Copy link

@GeorgeTaveras1231 GeorgeTaveras1231 commented Jan 2, 2024

NOTE: I need this code for one of my projects, so it includes changes from my other PR: #217. This is for convenience so that I can easily install a fork of this repo and get all of the fixes I need. The relevant list of changes can be observed here

With that said, it may make sense to review that PR first, and block this one until that one is merged. I will follow up and clean up this branch to only include the relevant changes once that is done.

Goal

  1. My goal with this PR is to customize the resolve function for CSS modules. I thought it was reasonable to allow customizing postcss-modules entirely. See the full list of options here. Some options may not make sense in this context (such as the scopedNameFormat) but others like exportGlobals and scopeBehaviour may be useful for consumers.

At the moment, this library has its own logic for manipulating the naming convention, this can be delegated to postcss-modules #localsConvention option.

Update: I no longer need the ability to customize the resolve option. The new IdentityObjProxyLoader removes the need to customize the resolution logic as all CSS Module imports are resolved and stubbed. It may still be useful to allow customizing the postcss-modules options, but I'd be okay with rolling back that change.

George Taveras added 8 commits January 2, 2024 14:08
- Handle all errors centrally and properly set process exit code
- Use console.warn where appropriate to funnel messaged to stderr
- Do not ignore errors when loading config file
This stops ignoring errors generated by the sass compiler (as well as
any other potential errors generated during the process of writing to
disk). Instead, errors are now always thrown to the top of the stack,
they are logged, and properly set the exit code. Any errors that include
file info are assumed to be sass errors, and some additional formatting
applied to improve debuggability.
This also changes the TS target to ES2022 to support the second argument of the Error
constructor.
@GeorgeTaveras1231
Copy link
Author

@skovy I noticed a subtle issue in the current implementation of source-to-class-names.

It parses a CSS file, resolves CSS Module imports, then it recursively processes CSS Module imports.

For example:

In the consumer's repo, one may have a SCSS file as

In main.scss

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

In some-other.scss

@use '@some-aliased-module' as m;

@include m.some-mixin-that-produces-some-class;

When postcss-modules encounters some-other.scss, it can't tell that it produces a class because it has to parse it as scss.

I'm not sure what happens at the moment, this probably produces a silent failure (something that I'm attempting to fix in #217). Also, in this example, it probably is ok beause we don't need some-class to produce the final types -- but it probably isn't great because it hides configuration problems, as the code is not being processed how it is intended to be.

I think the fix would be to pass a custom Loader to postcss-modules that handles scss imports.

Additionally, this can be leveraged to improve type safety. The proper types for main.scss should be

import {Styles as SomeOtherScssStyles} from './some-other.scss'
type Styles = {
  'my-class': SomeOtherScssStyles['some-class']
}

or some assertion that ensures that class exists (excuse my poor man's TS assertions 😅 )

import {Styles as SomeOtherScssStyles} from './some-other.scss'

type Assert<T extends 'yes'> = unknown;
type Assertion1 = Assert<SomeOtherScssStyles['some-class'] extends string ? 'yes' : 'no'>

type Styles = {
  'my-class':string
}

@GeorgeTaveras1231
Copy link
Author

GeorgeTaveras1231 commented Jan 3, 2024

About my last comment, I actually encountered this issue 😢 . When properly raising errors, this is preventing my build from working. I will try to fix this in this branch.

George Taveras added 2 commits January 3, 2024 13:42
Use an identity-obj-proxy to avoid loading downstream files. This solves
the problem called out here[^1], while minimizing disk access, at the
cost of issues with broken imports.

[^1]: skovy#218 (comment)
@GeorgeTaveras1231
Copy link
Author

GeorgeTaveras1231 commented Jan 3, 2024

@skovy Update: I fixed the issue I called out before by essentially creating a "noop" import whenever CSS modules import other CSS modules. I explained the solution in this commit: dd9a794

@skovy
Copy link
Owner

skovy commented Jan 15, 2024

@GeorgeTaveras1231 I don't fully follow, but we do expose the custom importer configuration: https://github.com/skovy/typed-scss-modules/tree/546dba68e624e6d632618b6a419892869fc63cb0?tab=readme-ov-file#importer. Have you tried this or does that work?

@skovy
Copy link
Owner

skovy commented Jan 15, 2024

I'm confused about what this PR is doing with changes from another branch, several updates, and bugs that seem to be unrelated. Please open focused issues or pull requests so we can discuss each item individually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants