Skip to content

Commit a7384c8

Browse files
author
Timothy Lindvall
committed
fix: PR feedback regarding errors.
- Ensure all new errors have a file identifier or source location info. - Update type for Compiled CSS files to include a FileIdentifier for definition data. For embedded data, a hash is appended to the path. - Update NodeJSImporter to include definition file identifiers.
1 parent 0d6e76a commit a7384c8

File tree

8 files changed

+118
-39
lines changed

8 files changed

+118
-39
lines changed

Diff for: ‎packages/@css-blocks/core/src/BlockParser/BlockFactory.ts

+10-3
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import { RawSourceMap } from "source-map";
66

77
import { Block } from "../BlockTree";
88
import { Options, ResolvedConfiguration, resolveConfiguration } from "../configuration";
9+
import { CssBlockError } from "../errors";
910
import { FileIdentifier, ImportedCompiledCssFile, ImportedFile, Importer } from "../importing";
1011
import { PromiseQueue } from "../util/PromiseQueue";
1112

@@ -313,23 +314,29 @@ export class BlockFactory {
313314
// NOTE: No need to run preprocessor - we assume that Compiled CSS has already been preprocessed.
314315
// Parse the definition file into an AST
315316
const definitionAst = this.postcssImpl.parse(file.definitionContents);
317+
const definitionFilepath = this.importer.filesystemPath(file.definitionIdentifier, this.configuration) || "<unknown filepath>";
316318

317319
// Parse the CSS contents into an AST
318320
const cssContentsAst = this.postcssImpl.parse(file.cssContents);
321+
const cssContentsFilepath = this.importer.filesystemPath(file.identifier, this.configuration) || "<unknown filepath>";
319322

320323
// TODO: Sourcemaps?
321324

322325
// Sanity check! Did we actually get contents for both ASTs?
323326
if (!definitionAst || !definitionAst.nodes) {
324-
throw new Error(`Unable to parse definition file into AST!\nIdentifier: ${file.identifier}`);
327+
throw new CssBlockError(`Unable to parse definition file into AST!`, {
328+
filename: definitionFilepath,
329+
});
325330
}
326331

327332
if (!cssContentsAst || !cssContentsAst.nodes) {
328-
throw new Error(`Unable to parse CSS contents into AST!\nIdentifier: ${file.identifier}`);
333+
throw new CssBlockError(`Unable to parse CSS contents into AST!`, {
334+
filename: cssContentsFilepath,
335+
});
329336
}
330337

331338
// Construct a Block out of the definition file.
332-
const block = this.parser.parseDefinitionSource(definitionAst, file.identifier, file.blockId);
339+
const block = this.parser.parseDefinitionSource(definitionAst, file.definitionIdentifier, file.blockId);
333340

334341
// Merge the rules from the CSS contents into the Block.
335342
// TODO: Actually merge the CSS rules in. (^_^")

Diff for: ‎packages/@css-blocks/core/src/BlockParser/BlockParser.ts

+3-3
Original file line numberDiff line numberDiff line change
@@ -84,14 +84,14 @@ export class BlockParser {
8484
// Assert that the block-id rule in :scope is declared and matches
8585
// header comment in Compiled CSS.
8686
debug(" - Assert Block IDs Match");
87-
await assertBlockIdsMatch(root, debugIdent, expectedId);
87+
await assertBlockIdsMatch(block, configuration, root, sourceFile, expectedId);
8888
debug(" - Assert Block Class Declared");
89-
await assertBlockClassDeclared(root, debugIdent);
89+
await assertBlockClassDeclared(block, configuration, root, sourceFile);
9090
} else {
9191
// If not a definition file, it shouldn't have rules that can
9292
// only be in definition files.
9393
debug(" - Disallow Definition-Only Declarations");
94-
await disallowDefinitionRules(configuration, root, sourceFile);
94+
await disallowDefinitionRules(block, configuration, root, sourceFile);
9595
}
9696
// Throw if we encounter any `!important` decls.
9797
debug(` - Disallow Important`);

Diff for: ‎packages/@css-blocks/core/src/BlockParser/features/assert-block-class-declared.ts

+28-6
Original file line numberDiff line numberDiff line change
@@ -1,28 +1,50 @@
11
import { postcss } from "opticss";
22

33
import { CLASS_NAME_IDENT } from "../../BlockSyntax";
4+
import { Block } from "../../BlockTree";
5+
import { Configuration } from "../../configuration";
46
import * as errors from "../../errors";
57
import { FileIdentifier } from "../../importing";
8+
import { sourceRange } from "../../SourceLocation";
69
import { stripQuotes } from "../utils";
710

8-
export async function assertBlockClassDeclared(root: postcss.Root, identifier: FileIdentifier): Promise<postcss.Root> {
11+
export async function assertBlockClassDeclared(block: Block, configuration: Configuration, root: postcss.Root, identifier: FileIdentifier): Promise<postcss.Root> {
912
let foundClassDecl = false;
13+
let scopeNode;
1014

1115
root.walkRules(":scope", (rule) => {
16+
scopeNode = rule;
1217
rule.walkDecls("block-class", (decl) => {
1318
const classVal = stripQuotes(decl.value);
1419
if (!CLASS_NAME_IDENT.exec(classVal)) {
15-
throw new errors.InvalidBlockSyntax(
16-
`Illegal block class. '${decl.value}' is not a legal CSS identifier.\nIdentifier: ${identifier}`,
20+
block.addError(
21+
new errors.InvalidBlockSyntax(
22+
`Illegal block class. '${decl.value}' is not a legal CSS identifier.`,
23+
sourceRange(configuration, root, identifier, decl),
24+
),
1725
);
1826
}
1927
});
2028
});
2129

2230
if (!foundClassDecl) {
23-
throw new errors.InvalidBlockSyntax(
24-
`Expected block-class to be declared in definition data.\nIdentifier: ${identifier}`,
25-
);
31+
if (scopeNode) {
32+
block.addError(
33+
new errors.InvalidBlockSyntax(
34+
`Expected block-class to be declared on :scope node.`,
35+
sourceRange(configuration, root, identifier, scopeNode),
36+
),
37+
);
38+
} else {
39+
block.addError(
40+
new errors.InvalidBlockSyntax(
41+
`Expected block-class to be declared on :scope node.`,
42+
{
43+
filename: identifier,
44+
},
45+
),
46+
);
47+
}
2648
}
2749

2850
return root;

Diff for: ‎packages/@css-blocks/core/src/BlockParser/features/assert-block-ids-match.ts

+29-11
Original file line numberDiff line numberDiff line change
@@ -1,31 +1,49 @@
11
import { postcss } from "opticss";
22

3+
import { Block } from "../../BlockTree";
4+
import { Configuration } from "../../configuration";
35
import * as errors from "../../errors";
46
import { FileIdentifier } from "../../importing";
7+
import { sourceRange } from "../../SourceLocation";
58
import { stripQuotes } from "../utils";
69

7-
export async function assertBlockIdsMatch(root: postcss.Root, identifier: FileIdentifier, expected?: string): Promise<postcss.Root> {
10+
export async function assertBlockIdsMatch(block: Block, configuration: Configuration, root: postcss.Root, identifier: FileIdentifier, expected?: string): Promise<postcss.Root> {
811
let foundIdDecl = false;
9-
10-
if (!expected) {
11-
throw new Error(`No expected ID provided.\nIdentifier: ${identifier}`);
12-
}
12+
let scopeNode;
1313

1414
root.walkRules(":scope", (rule) => {
15+
scopeNode = rule;
1516
rule.walkDecls("block-id", (decl) => {
16-
if (stripQuotes(decl.value) !== expected) {
17-
throw new errors.InvalidBlockSyntax(
18-
`Expected block-id property in definition data to match header in Compiled CSS.\nIdentifier: ${identifier}`,
17+
if (expected && stripQuotes(decl.value) !== expected) {
18+
block.addError(
19+
new errors.InvalidBlockSyntax(
20+
`Expected block-id property in definition data to match header in Compiled CSS.`,
21+
sourceRange(configuration, root, identifier, decl),
22+
),
1923
);
2024
}
2125
foundIdDecl = true;
2226
});
2327
});
2428

2529
if (!foundIdDecl) {
26-
throw new errors.InvalidBlockSyntax(
27-
`Expected block-id to be declared in definition data.\nIdentifier: ${identifier}`,
28-
);
30+
if (scopeNode) {
31+
block.addError(
32+
new errors.InvalidBlockSyntax(
33+
`Expected block-id to be declared in definition's :scope rule.`,
34+
sourceRange(configuration, root, identifier, scopeNode),
35+
),
36+
);
37+
} else {
38+
block.addError(
39+
new errors.InvalidBlockSyntax(
40+
`Expected block-id to be declared in definition's :scope rule.`,
41+
{
42+
filename: identifier,
43+
},
44+
),
45+
);
46+
}
2947
}
3048

3149
return root;

Diff for: ‎packages/@css-blocks/core/src/BlockParser/features/disallow-dfn-rules.ts

+12-7
Original file line numberDiff line numberDiff line change
@@ -1,21 +1,26 @@
11
import { postcss } from "opticss";
22

3+
import { Block } from "../../BlockTree";
34
import { Configuration } from "../../configuration";
45
import * as errors from "../../errors";
56
import { sourceRange } from "../../SourceLocation";
67

7-
export async function disallowDefinitionRules(configuration: Configuration, root: postcss.Root, file: string): Promise<postcss.Root> {
8+
export async function disallowDefinitionRules(block: Block, configuration: Configuration, root: postcss.Root, file: string): Promise<postcss.Root> {
89
root.walkRules((rule) => {
910
rule.walkDecls((decl) => {
1011
if (decl.prop === "block-id") {
11-
throw new errors.InvalidBlockSyntax(
12-
`block-id is disallowed in source block files.`,
13-
sourceRange(configuration, root, file, decl),
12+
block.addError(
13+
new errors.InvalidBlockSyntax(
14+
`block-id is disallowed in source block files.`,
15+
sourceRange(configuration, root, file, decl),
16+
),
1417
);
1518
} else if (decl.prop === "block-class") {
16-
throw new errors.InvalidBlockSyntax(
17-
`block-id is disallowed in source block files.`,
18-
sourceRange(configuration, root, file, decl),
19+
block.addError(
20+
new errors.InvalidBlockSyntax(
21+
`block-id is disallowed in source block files.`,
22+
sourceRange(configuration, root, file, decl),
23+
),
1924
);
2025
}
2126
});

Diff for: ‎packages/@css-blocks/core/src/BlockParser/features/discover-name.ts

+9-5
Original file line numberDiff line numberDiff line change
@@ -17,18 +17,22 @@ export async function discoverName(configuration: Configuration, root: postcss.R
1717
);
1818
}
1919

20-
defaultName = decl.value;
20+
defaultName = decl.value.trim();
2121

2222
});
2323
});
2424

2525
// We expect to have a block name by this point. Either we should have found one in the source
26-
// or inferred one from the filename. Definition files must include a blokc-name.
27-
if (!defaultName || defaultName.trim() === "") {
26+
// or inferred one from the filename. Definition files must include a block-name.
27+
if (!defaultName) {
2828
if (isDfnFile) {
29-
throw new errors.InvalidBlockSyntax(`block-name is expected to be declared in definition file's :scope rule.\nIdentifier: ${file}`);
29+
throw new errors.InvalidBlockSyntax(`block-name is expected to be declared in definition file's :scope rule.`, {
30+
filename: file,
31+
});
3032
} else {
31-
throw new errors.CssBlockError(`Unable to find or infer a block name.\nIdentifier: ${file}`);
33+
throw new errors.CssBlockError(`Unable to find or infer a block name.`, {
34+
filename: file,
35+
});
3236
}
3337
}
3438
return defaultName;

Diff for: ‎packages/@css-blocks/core/src/importing/Importer.ts

+7
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,13 @@ export interface ImportedCompiledCssFile {
119119
*/
120120
blockId: string;
121121

122+
/**
123+
* A unique identifier (probably an absolute filesystem path) for the block's definition
124+
* data. Even if the data is embedded in the same file as the Compiled CSS, this should
125+
* be distinct from the Compiled CSS identifier.
126+
*/
127+
definitionIdentifier: string;
128+
122129
/**
123130
* The contents of the block definition. If this was embedded base64 data, it will
124131
* have been decoded into a string. If this was an external file, the file's

Diff for: ‎packages/@css-blocks/core/src/importing/NodeJsImporter.ts

+20-4
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@ const debug = debugGenerator("css-blocks:importer");
1414

1515
const DEFAULT_MAIN = "blocks/index.block.css";
1616

17+
const EMBEDDED_DEFINITION_TAG = "#blockDefinitionURL";
18+
1719
export interface CSSBlocksPackageMetadata {
1820
"css-blocks"?: {
1921
main?: string;
@@ -110,6 +112,10 @@ export class NodeJsImporter extends BaseImporter {
110112
}
111113

112114
filesystemPath(identifier: FileIdentifier, _options: ResolvedConfiguration): string | null {
115+
if (identifier.endsWith(EMBEDDED_DEFINITION_TAG)) {
116+
const amendedId = identifier.replace(EMBEDDED_DEFINITION_TAG, "");
117+
return path.isAbsolute(amendedId) && existsSync(amendedId) ? `${amendedId}${EMBEDDED_DEFINITION_TAG}` : null;
118+
}
113119
return path.isAbsolute(identifier) && existsSync(identifier) ? identifier : null;
114120
}
115121

@@ -132,6 +138,9 @@ export class NodeJsImporter extends BaseImporter {
132138
if (alias) {
133139
return path.join(alias.alias, path.relative(alias.path, identifier));
134140
}
141+
if (identifier.endsWith(EMBEDDED_DEFINITION_TAG)) {
142+
return `path.relative(config.rootDir, identifier.replace(EMBEDDED_DEFINITION_TAG, ""))${EMBEDDED_DEFINITION_TAG}`;
143+
}
135144
return path.relative(config.rootDir, identifier);
136145
}
137146

@@ -145,28 +154,34 @@ export class NodeJsImporter extends BaseImporter {
145154
// follow, or embedded data.
146155
const dfnUrl = segmentedContents.definitionUrl;
147156
let dfnData: string | null = null;
157+
let definitionIdentifier: FileIdentifier | null = null;
148158
if (!isDefinitionUrlValid(dfnUrl)) {
149159
throw new Error(`Definition URL in Compiled CSS file is invalid.\nFile Identifier: ${identifier}\nDefinition URL: ${dfnUrl}`);
150160
}
151161
if (dfnUrl.startsWith("data:")) {
152162
// Parse this as embedded data.
153163
const [dfnHeader, dfnEncodedData] = dfnUrl.split(",");
164+
definitionIdentifier = `${identifier}${EMBEDDED_DEFINITION_TAG}`;
154165
if (dfnHeader === "data:text/css;base64") {
155166
dfnData = Buffer.from(dfnEncodedData, "base64").toString("utf-8");
156167
} else {
157-
throw new Error(`Definition data is in unsupported encoding or format. Embedded data must be in text/css;base64 format.\nFormat given: ${dfnHeader}`);
168+
throw new Error(`Definition data is in unsupported encoding or format. Embedded data must be in text/css;base64 format.\nFile Identifier: ${identifier}\nFormat given: ${dfnHeader}`);
158169
}
159170
} else {
160171
// Read in the definition data from the given path.
161-
const dfnIdentifier = this.identifier(identifier, dfnUrl, config);
172+
definitionIdentifier = this.identifier(identifier, dfnUrl, config);
162173
try {
163-
dfnData = await readFile(dfnIdentifier, "utf-8");
174+
dfnData = await readFile(definitionIdentifier, "utf-8");
164175
} catch (e) {
165176
throw new Error(`Definition URL in Compiled CSS file is invalid.\nFile Identifier: ${identifier}\nDefinition URL: ${dfnUrl}\nThrown error: ${e.message}`);
166177
}
167178
}
168179

169-
if (!dfnData || dfnData.trim() === "") {
180+
// Clean up definition data and location.
181+
dfnData = dfnData.trim();
182+
definitionIdentifier = definitionIdentifier.trim();
183+
184+
if (!dfnData) {
170185
throw new Error("Could not parse or read definition data.");
171186
}
172187

@@ -177,6 +192,7 @@ export class NodeJsImporter extends BaseImporter {
177192
cssContents: segmentedContents.blockCssContents,
178193
blockId: segmentedContents.blockId,
179194
definitionContents: dfnData,
195+
definitionIdentifier,
180196
};
181197
} else {
182198
return {

0 commit comments

Comments
 (0)