Skip to content
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 2 additions & 14 deletions lint/linter/test-schema.ts
Original file line number Diff line number Diff line change
@@ -1,27 +1,15 @@
/* This file is a part of @mdn/browser-compat-data
* See LICENSE file for more information. */

import { Ajv } from 'ajv';
import ajvErrors from 'ajv-errors';
import ajvFormats from 'ajv-formats';
import betterAjvErrors from 'better-ajv-errors';

import { createAjv } from '../../scripts/lib/ajv.js';
import { Linter, Logger, LinterData } from '../utils.js';

import compatDataSchema from './../../schemas/compat-data.schema.json' with { type: 'json' };
import browserDataSchema from './../../schemas/browsers.schema.json' with { type: 'json' };

const ajv = new Ajv({ allErrors: true });
// We use 'fast' because as a side effect that makes the "uri" format more lax.
// By default the "uri" format rejects ① and similar in URLs.
ajvFormats.default(ajv, { mode: 'fast' });
// Allow for custom error messages to provide better directions for contributors
ajvErrors.default(ajv);

// Define keywords for schema->TS converter
ajv.addKeyword('tsEnumNames');
ajv.addKeyword('tsName');
ajv.addKeyword('tsType');
const ajv = createAjv();

export default {
name: 'JSON Schema',
Expand Down
4 changes: 1 addition & 3 deletions schemas/browsers.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,8 @@
"$ref": "#/definitions/browser_statement"
},
"minProperties": 1,
"maxProperties": 1,
"errorMessage": {
"minProperties": "A browser must be described within the file.",
"maxProperties": "Each browser JSON file may only describe one browser."
"minProperties": "A browser must be described within the file."
},
"tsType": "Record<BrowserName, BrowserStatement>"
},
Expand Down
34 changes: 34 additions & 0 deletions scripts/build/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import fs from 'node:fs/promises';
import { relative } from 'node:path';
import { fileURLToPath } from 'node:url';

import betterAjvErrors from 'better-ajv-errors';
import esMain from 'es-main';
import stringify from 'fast-json-stable-stringify';
import { compareVersions } from 'compare-versions';
Expand All @@ -13,6 +14,9 @@ import { marked } from 'marked';
import { InternalSupportStatement } from '../../types/index.js';
import { BrowserName, CompatData, VersionValue } from '../../types/types.js';
import compileTS from '../generate-types.js';
import compatDataSchema from '../../schemas/compat-data.schema.json' with { type: 'json' };
import browserDataSchema from '../../schemas/browsers.schema.json' with { type: 'json' };
import { createAjv } from '../lib/ajv.js';
import { walk } from '../../utils/index.js';
import { WalkOutput } from '../../utils/walk.js';
import bcd from '../../index.js';
Expand Down Expand Up @@ -219,6 +223,35 @@ export const createDataBundle = async (): Promise<CompatData> => {
};
};

/**
* Validates the given data against the schema.
* @param data - The data to validate.
*/
const validate = (data: CompatData) => {
const ajv = createAjv();

for (const [key, value] of Object.entries(data)) {
if (key === '__meta') {
// Not covered by the schema.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See also 😒 #17574

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See also: #27685 (comment)

continue;
}

const schema = key === 'browsers' ? browserDataSchema : compatDataSchema;
const data = { [key]: value };
if (!ajv.validate(schema, data)) {
const errors = ajv.errors || [];
if (!errors.length) {
console.error(`${key} data failed validation with unknown errors!`);
}
// Output messages by one since better-ajv-errors wrongly joins messages
// (see https://github.com/atlassian/better-ajv-errors/pull/21)
errors.forEach((e) => {
console.error(betterAjvErrors(schema, data, [e], { indent: 2 }));
});
}
}
};

/* c8 ignore start */

/**
Expand All @@ -227,6 +260,7 @@ export const createDataBundle = async (): Promise<CompatData> => {
const writeData = async () => {
const dest = new URL('data.json', targetdir);
const data = await createDataBundle();
validate(data);
Copy link
Contributor Author

@caugner caugner Aug 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

If validation fails, the validation errors are logged via console.error, but we don't bail out, to avoid disruption for contributors in the unlikely event that this ever happens on main. In either case, these errors won't get unnoticed, because they will appear on every npm install (via the prepare script).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that a PR that breaks schema validation won't fail the build? I can kinda see the case for never failing during the prepare script, but it'd be nice if there were a test that does fail. I won't block on this though, as it would be a step up from what we have.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about checking process.env.CI and failing if we're in CI?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would help for local development, but not pull request checks. Maybe check if GITHUB_REF is refs/heads/main and, if true, convert errors to warnings, else do nothing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In pull request checks, we always have CI=true in the environment, so we would notice it in a PR before merging. (In fact, the PR check would fail, preventing a merge.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's tackle this afterwards.

await fs.writeFile(dest, stringify(data));
logWrite(dest, 'data');
};
Expand Down
23 changes: 23 additions & 0 deletions scripts/lib/ajv.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import { Ajv } from 'ajv';
import ajvErrors from 'ajv-errors';
import ajvFormats from 'ajv-formats';

/**
* Returns a new pre-configured Ajv instance.
* @returns the Ajv instance.
*/
export const createAjv = (): Ajv => {
const ajv = new Ajv({ allErrors: true });
// We use 'fast' because as a side effect that makes the "uri" format more lax.
// By default the "uri" format rejects ① and similar in URLs.
ajvFormats.default(ajv, { mode: 'fast' });
// Allow for custom error messages to provide better directions for contributors
ajvErrors.default(ajv);

// Define keywords for schema->TS converter
ajv.addKeyword('tsEnumNames');
ajv.addKeyword('tsName');
ajv.addKeyword('tsType');

return ajv;
};