feat: support non identifier classnames by apple-yagi · Pull Request #309 · mizdra/css-modules-kit

apple-yagi · 2026-01-21T14:51:57Z

Background

At a social gathering after a certain study session held in Fukuoka the other day, the topic of css-modules-kit came up. At that time, I heard that css-modules-kit does not allow kebab-case, so I couldn't use it. I investigated whether it was not allowed, and found that it was, so I submitted a PR.

Description

I have modified it to generate type definitions using string literals, referring to #177. This allows defining classnames that are not JS identifiers.
Since namedExports can only use JS identifiers, this will only support defaultExports.

As an implementation issue, the logic has become complex because we needed to manage mappings doubly (one for definition jumps and one for renaming) (this change caused various bugs).
While I believe this feature addition is good for the further adoption of css-modules-kit, I also want to avoid making the code so complex that it becomes unmaintainable in the long run. I would like to respect mizdra's opinion on this 🙏

Related issues

#176

AI Assistance

This PR was written with assistance from Codex (ChatGPT).

changeset-bot · 2026-01-21T14:52:01Z

⚠️ No Changeset found

Latest commit: 51719ff

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.

This PR includes no changesets

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

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

mizdra · 2026-01-22T01:56:39Z

Great work! This is a big change. I'll review it carefully. It may take some time.

mizdra · 2026-01-22T09:53:24Z

FYI: CSS Modules Kit has flaky tests (#301). Even if your implementation is correct, CI may fail.

apple-yagi · 2026-01-22T09:58:11Z

Looking back at the code, I noticed there was unnecessary code and deleted it 🙏

mizdra · 2026-01-24T14:54:27Z

@apple-yagi I've made some changes to the main branch—could you please merge or rebase the main branch?

Fix flaky test (packages/ts-plugin/e2e-test/file-operation.test.ts > updating file) #314
- Some flaky tests have been fixed
Fix incorrect Definition Preview #315
- feat: support non identifier classnames #309 appears to have contained bug fixes performed by Codex.
- It's not ideal to combine bug fixes and feature additions in a single Pull Request, so I addressed the bug in Fix incorrect Definition Preview #315 instead.

mizdra · 2026-01-24T17:14:53Z

While I believe this feature addition is good for the further adoption of css-modules-kit, I also want to avoid making the code so complex that it becomes unmaintainable in the long run. I would like to respect mizdra's opinion on this 🙏

It certainly looks like a bit of complex code. I'll think deeply about whether there's a way to replace it with simpler code.

apple-yagi · 2026-01-25T13:11:44Z

@mizdra I believe all functions are now working correctly after fixing the bug in rename.
I don't think I can improve the quality of this code further, so I apologize, but I'll leave the rest to you.
Good luck! 🙇

…#309 This is the issue fixed in mizdra#317.

mizdra · 2026-01-25T16:42:17Z

+  return result;
+}
+
 export function getDefinitionAndBoundSpan(


Honestly, I still don't understand what this function is trying to do. Is it doing something that contributes to supporting non-identifier classnames?

When type definitions are made into string literals, tsserver cannot return definitions (it might be a bug in some tool, but I don't know). Therefore, I am manually mapping the definitions(I had Codex write the code).

Hmm, shouldn't we investigate that bug first? If it's a dependency bug and fixable, fixing it is the best approach.

If the dependency bug is difficult to fix or would take a long time to resolve, I think implementing a workaround to bypass it is worthwhile. Otherwise, I'd prefer to avoid implementing a complex workaround.

I'll investigate this bug. At this stage, I suspect it's a bug in tsserver or Volar.js.

Yes, I agree.
While organizing the discussion, I thought of another implementation method, so I'll create a new Pull Request and commit step by step. When defining types with string literals, I feel that single quotes are also recognized as part of the type. That might be a specification of tsserver (I briefly read the tsserver implementation but couldn't quite understand it).
With that in mind, I'll try implementing it using tsserver.

Sharing progress.

I also opened a PR to support non-js identifier tokens.

[PoC] Support non js identifier token #322

Unlike @apple-yagi's PR, this one addresses several edge cases.

83d1224

Currently, this PR also breaks definitions and references results.

While implementing the above Pull Request, I found and fixed several bugs. I also performed some refactoring.

Since the main branch was updated, @apple-yagi's PR is now in conflict. Sorry about that!

Investigating the issue where definitions and references results break in the following repository:

https://github.com/mizdra/typescript-volar-span-comparison

Currently, I believe we need to create an issue in Volar.js and submit a patch. I plan to do that later :)

Thank you for sharing! The findings in https://github.com/mizdra/typescript-volar-span-comparison are very clear, and I was able to understand what's happening. Great work!

@apple-yagi I investigated Volar.js's behavior further. It appears Volar.js intentionally does not support overlapping mapping ranges.

https://github.com/mizdra/volar-single-quote-span-problem?tab=readme-ov-file#now-lets-generate-mappings-for-both-a_1-and-a_1

While we could submit a feature request, it's uncertain whether it would be accepted. Therefore, implementing a workaround in CSS Modules Kit seems the best approach.

I considered several workarounds, but I think the fourth solution in the link below, "Using multiple mapping objects" is the best approach.

https://github.com/mizdra/volar-single-quote-span-problem?tab=readme-ov-file#now-lets-generate-mappings-for-both-a_1-and-a_1

I implemented it in #330. It passes all test cases. I plan to merge this.

...By the way, after creating #330, I noticed that #320 also adopted same solution! I should have reviewed the pull request sooner 🙇 Did the Codex come up with that solution too? If so, that's amazing.

Even before using Codex, I had an intuitive feeling that double-mapping would work (I later realized that whether quotes are included or not changes when using "go to definition" and "rename symbol" functions: https://github.com/microsoft/TypeScript/blob/022800b0472ecfb8d94ce10e2b51cf01acede27/src/services/rename.ts#L219-L228).

Conversely, in this PR, Codex suggested that it would be better to stop double-mapping midway, so I accepted it. I made the wrong choice...

I apologize if I confused you, mizdra-san, but I'm glad the feature was implemented! Thank you very much!

I knew Volar.js was behaving strangely, but I couldn't figure out the specific cause until I read mizdra's report, which helped me understand it. I never thought there would be issues similar to ours, so I didn't even bother looking for them 😅

That's amazing intuition! I struggled to come up with that idea myself.

Anyway, I'm glad we got the feature implemented. Thanks for building the PoC!

mizdra · 2026-01-25T16:59:34Z

I refactored dts-generator.ts to avoid breaking the code (1340313...12f5b4b). Additionally, I fixed the regression (c4dbe5b). As a result, it seems adding new mappings is no longer necessary.

By the way, you explained the following in #309 (comment):

As an implementation issue, the logic has become complex because we needed to manage mappings doubly

What does "managing mappings doubly" mean? Does it mean one more element is added to the mapping array? Looking at the changes in dts-generator.ts, it doesn't appear that any additional elements are being added to the mapping array.

@mizdra Sorry, Initially, when I submitted the PR, I thought that both quoted and unquoted mappings were necessary for string literals like styles['a-1'], because tsserver returns spans including quotes

css-modules-kit/packages/core/src/dts-generator.ts

Lines 262 to 275 in 96d2d6f

text += ` '`;

mapping.sourceOffsets.push(token.loc.start.offset);

const quoteStart = base + 2;

const keyStart = base + 3;

mapping.generatedOffsets.push(quoteStart);

mapping.lengths.push(token.name.length);

// Include quotes and `:` so string-literal ranges map correctly.

mapping.generatedLengths!.push(token.name.length + 3);

// Map the inner content so rename ranges without quotes still resolve.

mapping.sourceOffsets.push(token.loc.start.offset);

mapping.generatedOffsets.push(keyStart);

mapping.lengths.push(token.name.length);

mapping.generatedLengths!.push(token.name.length);

text += `${token.name}': '' as readonly string,\n`;

.
However, I realized that go-to-definition works even without double mapping because getDefinitionAndBoundSpan normalizes def.name and replaces it with the CSS location (https://github.com/apple-yagi/css-modules-kit/blob/c4dbe5b554281a3501ec0cb6544cbc62875e6ba6/packages/ts-plugin/src/language-service/feature/definition-and-bound-span.ts#L110-L132). Therefore, I removed it.

Incidentally, performing double mapping allows tsserver's getDefinitionAndBoundSpan to work partially correctly. For example, jumps for styles.a_1; or styles['a-1']; work correctly (this was my original idea). However, bugs occur with duplicate class names like styles.a_2, @import, @value ... from '...';, and symbol renaming, making the implementation increasingly complex.

apple-yagi force-pushed the feat/support-non-identifier-classnames branch from 96d2d6f to 3e6cbfb Compare January 21, 2026 14:57

mizdra added the Type: Feature New Feature label Jan 22, 2026

mizdra reviewed Jan 24, 2026

View reviewed changes

Comment thread packages/ts-plugin/e2e-test/feature/rename-symbol.test.ts

apple-yagi force-pushed the feat/support-non-identifier-classnames branch 3 times, most recently from 81f5cfb to 76ca2ba Compare January 25, 2026 12:56

feat: support non identifier classnames

1340313

apple-yagi force-pushed the feat/support-non-identifier-classnames branch from 76ca2ba to 1340313 Compare January 25, 2026 12:59

mizdra added 5 commits January 26, 2026 01:06

test: refactor test

8e5806f

refactor: refactoring to make changes easier to understand

cf34884

refactor: refactor test

1d4f1f3

refactor: generatedLengths can be omitted when it is equal to length

12f5b4b

fix: fix regression that occurred during the implementation of mizdra…

c4dbe5b

…#309 This is the issue fixed in mizdra#317.

mizdra reviewed Jan 25, 2026

View reviewed changes

refactor: remove dedupeDefinitions

51719ff

apple-yagi mentioned this pull request Jan 27, 2026

feat: support non identifier classnames 2 #320

Closed

mizdra mentioned this pull request Feb 4, 2026

Support non-JavaScript identifier token in default export #330

Merged

apple-yagi closed this Feb 4, 2026

apple-yagi deleted the feat/support-non-identifier-classnames branch February 4, 2026 21:37

	text += ` '`;
	mapping.sourceOffsets.push(token.loc.start.offset);
	const quoteStart = base + 2;
	const keyStart = base + 3;
	mapping.generatedOffsets.push(quoteStart);
	mapping.lengths.push(token.name.length);
	// Include quotes and `:` so string-literal ranges map correctly.
	mapping.generatedLengths!.push(token.name.length + 3);
	// Map the inner content so rename ranges without quotes still resolve.
	mapping.sourceOffsets.push(token.loc.start.offset);
	mapping.generatedOffsets.push(keyStart);
	mapping.lengths.push(token.name.length);
	mapping.generatedLengths!.push(token.name.length);
	text += `${token.name}': '' as readonly string,\n`;

Uh oh!

Conversation

apple-yagi commented Jan 21, 2026

Background

Description

Related issues

AI Assistance

Uh oh!

changeset-bot Bot commented Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

mizdra commented Jan 22, 2026

Uh oh!

mizdra commented Jan 22, 2026

Uh oh!

apple-yagi commented Jan 22, 2026

Uh oh!

mizdra commented Jan 24, 2026

Uh oh!

Uh oh!

mizdra commented Jan 24, 2026

Uh oh!

apple-yagi commented Jan 25, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mizdra Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

changeset-bot Bot commented Jan 21, 2026 •

edited

Loading

mizdra Feb 4, 2026 •

edited

Loading