Skip to content

Commit 15dcba8

Browse files
mizdramizdra
authored
Improve non-JS identifier error message: Phase 2 (#222)
* remove the logic for invalid name diagnostics * report diagnostics for invalid name in checker * do not create types for invalid name * update E2E test * add changelog
1 parent 6978cf4 commit 15dcba8

14 files changed

Lines changed: 202 additions & 211 deletions

‎.changeset/spicy-wings-hide.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@css-modules-kit/core': minor
3+
---
4+
5+
feat: improve non-JS identifier error message

‎packages/core/src/checker.test.ts‎

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,96 @@ function fakeLoc({ column }: { column: number }): Location {
3333
}
3434

3535
describe('checkCSSModule', () => {
36+
test('report diagnostics for invalid name as js identifier', () => {
37+
const args = prepareCheckerArgs([
38+
fakeCSSModule({
39+
fileName: '/a.module.css',
40+
localTokens: [fakeToken({ name: 'a-1', loc: fakeLoc({ column: 1 }) })],
41+
tokenImporters: [
42+
fakeAtValueTokenImporter({
43+
from: './b.module.css',
44+
fromLoc: fakeLoc({ column: 2 }),
45+
values: [
46+
fakeAtValueTokenImporterValue({ name: 'b-1', loc: fakeLoc({ column: 3 }) }),
47+
fakeAtValueTokenImporterValue({
48+
name: 'b-2',
49+
loc: fakeLoc({ column: 4 }),
50+
localName: 'a-2',
51+
localLoc: fakeLoc({ column: 5 }),
52+
}),
53+
],
54+
}),
55+
],
56+
}),
57+
fakeCSSModule({
58+
fileName: '/b.module.css',
59+
localTokens: [fakeToken({ name: 'b-1' }), fakeToken({ name: 'b-2' })],
60+
}),
61+
]);
62+
const diagnostics = checkCSSModule(
63+
args.cssModules[0],
64+
args.exportBuilder,
65+
args.matchesPattern,
66+
args.resolver,
67+
args.getCSSModule,
68+
);
69+
expect(diagnostics).toMatchInlineSnapshot(`
70+
[
71+
{
72+
"category": "error",
73+
"file": {
74+
"fileName": "/a.module.css",
75+
"text": "",
76+
},
77+
"length": 0,
78+
"start": {
79+
"column": 1,
80+
"line": 1,
81+
},
82+
"text": "css-modules-kit does not support invalid names as JavaScript identifiers.",
83+
},
84+
{
85+
"category": "error",
86+
"file": {
87+
"fileName": "/a.module.css",
88+
"text": "",
89+
},
90+
"length": 0,
91+
"start": {
92+
"column": 3,
93+
"line": 1,
94+
},
95+
"text": "css-modules-kit does not support invalid names as JavaScript identifiers.",
96+
},
97+
{
98+
"category": "error",
99+
"file": {
100+
"fileName": "/a.module.css",
101+
"text": "",
102+
},
103+
"length": 0,
104+
"start": {
105+
"column": 4,
106+
"line": 1,
107+
},
108+
"text": "css-modules-kit does not support invalid names as JavaScript identifiers.",
109+
},
110+
{
111+
"category": "error",
112+
"file": {
113+
"fileName": "/a.module.css",
114+
"text": "",
115+
},
116+
"length": 0,
117+
"start": {
118+
"column": 5,
119+
"line": 1,
120+
},
121+
"text": "css-modules-kit does not support invalid names as JavaScript identifiers.",
122+
},
123+
]
124+
`);
125+
});
36126
test('report diagnostics for non-existing module', () => {
37127
const args = prepareCheckerArgs([
38128
fakeCSSModule({

‎packages/core/src/checker.ts‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,12 @@ import type {
44
CSSModule,
55
Diagnostic,
66
ExportBuilder,
7+
Location,
78
MatchesPattern,
89
Resolver,
910
TokenImporter,
1011
} from './type.js';
12+
import { isValidAsJSIdentifier } from './util.js';
1113

1214
export function checkCSSModule(
1315
cssModule: CSSModule,
@@ -18,6 +20,12 @@ export function checkCSSModule(
1820
): Diagnostic[] {
1921
const diagnostics: Diagnostic[] = [];
2022

23+
for (const token of cssModule.localTokens) {
24+
if (!isValidAsJSIdentifier(token.name)) {
25+
diagnostics.push(createInvalidNameAsJSIdentifiersDiagnostic(cssModule, token.loc));
26+
}
27+
}
28+
2129
for (const tokenImporter of cssModule.tokenImporters) {
2230
const from = resolver(tokenImporter.from, { request: cssModule.fileName });
2331
if (!from || !matchesPattern(from)) continue;
@@ -33,6 +41,12 @@ export function checkCSSModule(
3341
if (!exportRecord.allTokens.includes(value.name)) {
3442
diagnostics.push(createModuleHasNoExportedTokenDiagnostic(cssModule, tokenImporter, value));
3543
}
44+
if (!isValidAsJSIdentifier(value.name)) {
45+
diagnostics.push(createInvalidNameAsJSIdentifiersDiagnostic(cssModule, value.loc));
46+
}
47+
if (value.localName && !isValidAsJSIdentifier(value.localName)) {
48+
diagnostics.push(createInvalidNameAsJSIdentifiersDiagnostic(cssModule, value.localLoc!));
49+
}
3650
}
3751
}
3852
}
@@ -62,3 +76,13 @@ function createModuleHasNoExportedTokenDiagnostic(
6276
length: value.loc.end.offset - value.loc.start.offset,
6377
};
6478
}
79+
80+
function createInvalidNameAsJSIdentifiersDiagnostic(cssModule: CSSModule, loc: Location): Diagnostic {
81+
return {
82+
text: `css-modules-kit does not support invalid names as JavaScript identifiers.`,
83+
category: 'error',
84+
file: { fileName: cssModule.fileName, text: cssModule.text },
85+
start: { line: loc.start.line, column: loc.start.column },
86+
length: loc.end.offset - loc.start.offset,
87+
};
88+
}

‎packages/core/src/dts-creator.test.ts‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import type { CreateDtsHost } from './dts-creator.js';
33
import { createDts, type CreateDtsOptions } from './dts-creator.js';
44
import { fakeCSSModule } from './test/css-module.js';
55
import { fakeMatchesPattern, fakeResolver } from './test/faker.js';
6+
import { fakeAtValueTokenImporter, fakeAtValueTokenImporterValue, fakeToken } from './test/token.js';
67

78
const host: CreateDtsHost = {
89
resolver: fakeResolver(),
@@ -208,6 +209,38 @@ describe('createDts', () => {
208209
"
209210
`);
210211
});
212+
test('does not create types for invalid name as JS identifier', () => {
213+
expect(
214+
createDts(
215+
fakeCSSModule({
216+
localTokens: [fakeToken({ name: 'a-1', loc: fakeLoc(0) })],
217+
tokenImporters: [
218+
fakeAtValueTokenImporter({
219+
from: './b.module.css',
220+
fromLoc: fakeLoc(1),
221+
values: [
222+
fakeAtValueTokenImporterValue({ name: 'b-1', loc: fakeLoc(2) }),
223+
fakeAtValueTokenImporterValue({
224+
name: 'b_2',
225+
loc: fakeLoc(3),
226+
localName: 'a-2',
227+
localLoc: fakeLoc(4),
228+
}),
229+
],
230+
}),
231+
],
232+
}),
233+
host,
234+
options,
235+
).text,
236+
).toMatchInlineSnapshot(`
237+
"// @ts-nocheck
238+
declare const styles = {
239+
};
240+
export default styles;
241+
"
242+
`);
243+
});
211244
test('creates d.ts file with named exports', () => {
212245
expect(
213246
createDts(

‎packages/core/src/dts-creator.ts‎

Lines changed: 27 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import type { CSSModule, MatchesPattern, Resolver, Token, TokenImporter } from './type.js';
2+
import { isValidAsJSIdentifier } from './util.js';
23

34
export const STYLES_EXPORT_NAME = 'styles';
45

@@ -46,15 +47,34 @@ interface CreateDtsResult {
4647
* Create a d.ts file.
4748
*/
4849
export function createDts(cssModules: CSSModule, host: CreateDtsHost, options: CreateDtsOptions): CreateDtsResult {
49-
// Filter external files
50-
const tokenImporters = cssModules.tokenImporters.filter((tokenImporter) => {
51-
const resolved = host.resolver(tokenImporter.from, { request: cssModules.fileName });
52-
return resolved !== undefined && host.matchesPattern(resolved);
53-
});
50+
// Exclude tokens that are not valid as JS identifiers
51+
const localTokens = cssModules.localTokens.filter((token) => isValidAsJSIdentifier(token.name));
52+
const tokenImporters = cssModules.tokenImporters
53+
// Exclude imported tokens that are not valid as JS identifiers
54+
.map((tokenImporter) => {
55+
if (tokenImporter.type === 'value') {
56+
return {
57+
...tokenImporter,
58+
values: tokenImporter.values.filter(
59+
(value) =>
60+
isValidAsJSIdentifier(value.name) &&
61+
(value.localName === undefined || isValidAsJSIdentifier(value.localName)),
62+
),
63+
};
64+
} else {
65+
return tokenImporter;
66+
}
67+
})
68+
// Exclude token importers for external files
69+
.filter((tokenImporter) => {
70+
const resolved = host.resolver(tokenImporter.from, { request: cssModules.fileName });
71+
return resolved !== undefined && host.matchesPattern(resolved);
72+
});
73+
5474
if (options.namedExports) {
55-
return createNamedExportsDts(cssModules.localTokens, tokenImporters, options);
75+
return createNamedExportsDts(localTokens, tokenImporters, options);
5676
} else {
57-
return createDefaultExportDts(cssModules.localTokens, tokenImporters);
77+
return createDefaultExportDts(localTokens, tokenImporters);
5878
}
5979
}
6080

‎packages/core/src/parser/at-value-parser.test.ts‎

Lines changed: 1 addition & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -412,12 +412,10 @@ describe('parseAtValue', () => {
412412
`);
413413
});
414414
test('invalid', () => {
415-
const [atValue1, atValue2, atValue3, atValue4] = fakeAtValues(
415+
const [atValue1, atValue2] = fakeAtValues(
416416
fakeRoot(dedent`
417417
@value;
418418
@value a,,b from "test.css";
419-
@value non-js-ident-1: #000;
420-
@value non-js-ident-1, non-js-ident-2 as alias_1, a as non-js-ident-3 from "test.css";
421419
`),
422420
);
423421
expect(parseAtValue(atValue1!)).toMatchInlineSnapshot(`
@@ -498,70 +496,5 @@ describe('parseAtValue', () => {
498496
],
499497
}
500498
`);
501-
expect(parseAtValue(atValue3!)).toMatchInlineSnapshot(`
502-
{
503-
"diagnostics": [
504-
{
505-
"category": "error",
506-
"length": 14,
507-
"start": {
508-
"column": 8,
509-
"line": 3,
510-
},
511-
"text": "css-modules-kit does not support non-JavaScript identifier as value names.",
512-
},
513-
],
514-
}
515-
`);
516-
expect(parseAtValue(atValue4!)).toMatchInlineSnapshot(`
517-
{
518-
"atValue": {
519-
"from": "test.css",
520-
"fromLoc": {
521-
"end": {
522-
"column": 85,
523-
"line": 4,
524-
"offset": 150,
525-
},
526-
"start": {
527-
"column": 77,
528-
"line": 4,
529-
"offset": 142,
530-
},
531-
},
532-
"type": "valueImportDeclaration",
533-
"values": [],
534-
},
535-
"diagnostics": [
536-
{
537-
"category": "error",
538-
"length": 14,
539-
"start": {
540-
"column": 8,
541-
"line": 4,
542-
},
543-
"text": "css-modules-kit does not support non-JavaScript identifier as value names.",
544-
},
545-
{
546-
"category": "error",
547-
"length": 14,
548-
"start": {
549-
"column": 24,
550-
"line": 4,
551-
},
552-
"text": "css-modules-kit does not support non-JavaScript identifier as value names.",
553-
},
554-
{
555-
"category": "error",
556-
"length": 14,
557-
"start": {
558-
"column": 56,
559-
"line": 4,
560-
},
561-
"text": "css-modules-kit does not support non-JavaScript identifier as value names.",
562-
},
563-
],
564-
}
565-
`);
566499
});
567500
});

‎packages/core/src/parser/at-value-parser.ts‎

Lines changed: 0 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
import type { AtRule } from 'postcss';
22
import type { DiagnosticPosition, DiagnosticWithDetachedLocation, Location } from '../type.js';
3-
import { JS_IDENTIFIER_PATTERN } from '../util.js';
43

54
interface ValueDeclaration {
65
type: 'valueDeclaration';
@@ -84,17 +83,6 @@ export function parseAtValue(atValue: AtRule): ParseAtValueResult {
8483
column: start.column + name.length,
8584
offset: start.offset + name.length,
8685
};
87-
88-
if (!JS_IDENTIFIER_PATTERN.test(name)) {
89-
diagnostics.push({
90-
start: { line: start.line, column: start.column },
91-
length: name.length,
92-
text: `css-modules-kit does not support non-JavaScript identifier as value names.`,
93-
category: 'error',
94-
});
95-
continue;
96-
}
97-
9886
const result = { name, loc: { start, end } };
9987
if (localName === undefined) {
10088
values.push(result);
@@ -110,17 +98,6 @@ export function parseAtValue(atValue: AtRule): ParseAtValueResult {
11098
column: start.column + localName.length,
11199
offset: start.offset + localName.length,
112100
};
113-
114-
if (!JS_IDENTIFIER_PATTERN.test(localName)) {
115-
diagnostics.push({
116-
start: { line: start.line, column: start.column },
117-
length: localName.length,
118-
text: `css-modules-kit does not support non-JavaScript identifier as value names.`,
119-
category: 'error',
120-
});
121-
continue;
122-
}
123-
124101
values.push({ ...result, localName, localLoc: { start, end } });
125102
}
126103
} else {
@@ -177,17 +154,6 @@ export function parseAtValue(atValue: AtRule): ParseAtValueResult {
177154
column: start.column + name.length,
178155
offset: start.offset + name.length,
179156
};
180-
181-
if (!JS_IDENTIFIER_PATTERN.test(name)) {
182-
diagnostics.push({
183-
start: { line: start.line, column: start.column },
184-
length: name.length,
185-
text: `css-modules-kit does not support non-JavaScript identifier as value names.`,
186-
category: 'error',
187-
});
188-
return { diagnostics };
189-
}
190-
191157
const parsedAtValue: ValueDeclaration = {
192158
type: 'valueDeclaration',
193159
name,

0 commit comments

Comments
 (0)