feat: support non identifier classnames 2 #320
Conversation
|
packages/core/src/dts-generator.ts
Outdated
| if (isValidAsJSIdentifier(token.name)) { | ||
| mapping.generatedLengths!.push(token.name.length); | ||
| text += `${token.name}: '' as readonly string,\n`; | ||
| } else { | ||
| // Include quotes in the mapping for invalid JS identifiers | ||
| mapping.generatedLengths!.push(token.name.length + 2); | ||
| text += `'${token.name}': '' as readonly string,\n`; | ||
| } |
There was a problem hiding this comment.
We are branching to minimize the scope of impact.
| function isValidName(name: string, options: GenerateDtsOptions): boolean { | ||
| if (!isValidAsJSIdentifier(name)) return false; | ||
| function isValidName(name: string, options: GenerateDtsOptions, isTokenImport: boolean): boolean { | ||
| if ((options.namedExports || isTokenImport) && !isValidAsJSIdentifier(name)) return false; |
There was a problem hiding this comment.
We are branching to minimize the scope of impact.
apple-yagi
force-pushed
the
feat/support-non-identifier-classnames-2
branch
from
f868dcb to
9580878
Compare
| // TODO | ||
| // { | ||
| // name: 'e-1 in index.ts', | ||
| // file: iff.paths['index.ts'], | ||
| // line: 9, | ||
| // offset: 9, | ||
| // expected: [ | ||
| // { | ||
| // file: formatPath(iff.paths['index.ts']), | ||
| // locs: [{ start: { line: 9, offset: 9 }, end: { line: 9, offset: 12 } }], | ||
| // }, | ||
| // { | ||
| // file: formatPath(iff.paths['a.module.css']), | ||
| // locs: [{ start: { line: 6, offset: 2 }, end: { line: 6, offset: 5 } }], | ||
| // }, | ||
| // ], | ||
| // }, |
There was a problem hiding this comment.
@mizdra In the current implementation, renaming CSS fails when making symbol changes from tsx files (only non-identifiers).
The tsserver logs are as follows, including single quotes (because single quotes are included in mapping's generatedLength).
To solve this problem, I am considering adding processing to remove single quotes during renaming. Please refer to this: https://github.com/mizdra/css-modules-kit/blob/main/packages/ts-plugin/src/protocol-handler/rename.ts.
What do you think about this? Fundamentally, I'm unsure if it's strange that single quotes need to be included in generatedLength.
There was a problem hiding this comment.
tsserver log
Info 910 [13:01:32.171] request:
{
"seq": 187,
"type": "request",
"command": "rename",
"arguments": {
"file": "/Users/ryuya.yanagi/workspace/oss/css-modules-kit/examples/1-basic/src/a.tsx",
"line": 10,
"offset": 10,
"findInStrings": false,
"findInComments": false
}
}
Perf 911 [13:01:32.205] 187::rename: elapsed time (in milliseconds) 33.6918
Info 912 [13:01:32.205] response:
{
"seq": 0,
"type": "response",
"command": "rename",
"request_seq": 187,
"success": true,
"body": {
"info": {
"canRename": true,
"displayName": "'a-1'",
"fullDisplayName": "styles.'a-1'",
"kind": "property",
"kindModifiers": "declare",
"triggerSpan": {
"start": {
"line": 10,
"offset": 9
},
"end": {
"line": 10,
"offset": 12
}
}
},
"locs": [
{
"file": "/Users/ryuya.yanagi/workspace/oss/css-modules-kit/examples/1-basic/src/a.tsx",
"locs": [
{
"start": {
"line": 10,
"offset": 9
},
"end": {
"line": 10,
"offset": 12
}
}
]
},
{
"file": "/Users/ryuya.yanagi/workspace/oss/css-modules-kit/examples/1-basic/src/a.module.css",
"locs": [
{
"start": {
"line": 10,
"offset": 3
},
"end": {
"line": 10,
"offset": 5
}
}
]
}
]
}
}
packages/core/src/dts-generator.ts
Outdated
| text += `${token.name}: '' as readonly string,\n`; | ||
| } else { | ||
| // Include quotes in the mapping for invalid JS identifiers | ||
| mapping.generatedLengths!.push(token.name.length + 2); |
There was a problem hiding this comment.
What we currently know is that if we put the length including quotes into generatedLengths, languageService.getDefinitionAndBoundSpan works fine. However, since the type length includes single quotes, there is a difference in length between the type and the class name during a rename operation, causing the rename to fail. Therefore, when renaming, we need to remove the quotes from the type so that its length matches that of the class name.
If we don't include quotes in generatedLengths (i.e., if we remove generatedLengths), languageService.getDefinitionAndBoundSpan stops working entirely. This would necessitate implementing functionality equivalent to languageService.getDefinitionAndBoundSpan ourselves, similar to #309, which would complicate the implementation.
I am questioning why languageService.getDefinitionAndBoundSpan doesn't work correctly without including quotes, even though TypeScript Symbols themselves should not include quotes in the first place.
There was a problem hiding this comment.
It seems that when tsserver's getDefinitionAndBoundSpan encounters a type definition that is a string literal, the textSpan's length becomes 5. Therefore, it appears necessary to set generatedLengths to 5 to match this.
There was a problem hiding this comment.
Do not make implementing workarounds your first choice. You should first investigate the validity of the behavior of languageService.getDefinitionAndBoundSpan. Examine the behavior in both patterns: when combined with Volar.js and when not combined with Volar.js. You should isolate whether the behavior originates from Volar.js or tsserver.
Then, thoroughly investigate whether this behavior is a bug or by design. If it's a bug, create an issue and report it to the TypeScript or Volar.js maintainers. If it's by design, implement a workaround.
Fundamentally, implementing a workaround should be the last resort.
Right now, I am investigating whether this behavior is a bug or by design. Until this investigation is complete, I cannot evaluate the merits of your proposed workaround.
There was a problem hiding this comment.
Thank you for your comment. I unconsciously accepted the behavior of tsserver and volar.js 🙏
apple-yagi
force-pushed
the
feat/support-non-identifier-classnames-2
branch
from
83b5e99 to
49fa2e1
Compare
2 participants
#309 (comment)