DEV: Catch transformer errors and improve warnings (#28205)

This commit is contained in:
Sérgio Saquetim 2024-08-02 15:58:13 -03:00 committed by GitHub
parent 1b2e0e86f8
commit 4167862a05
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 287 additions and 38 deletions

View File

@ -1,5 +1,6 @@
import { DEBUG } from "@glimmer/env";
import { capitalize } from "@ember/string";
import { consolePrefix } from "discourse/lib/source-identifier";
import {
BEHAVIOR_TRANSFORMERS,
VALUE_TRANSFORMERS,
@ -15,6 +16,13 @@ export const transformerTypes = Object.freeze({
VALUE: "VALUE",
});
/**
* Test flag - disables throwing an exception if applying a transformer fails on tests
*
* @type {boolean}
*/
let skipApplyExceptionOnTests = false;
/**
* Valid core transformer names initialization.
*
@ -87,7 +95,7 @@ function _normalizeTransformerName(name, type) {
/**
* Adds a new valid transformer name.
*
* INTERNAL API: use pluginApi.addValueTransformerName instead.
* INTERNAL API: use pluginApi.addValueTransformerName or pluginApi.addBehaviorTransformerName instead.
*
* DO NOT USE THIS FUNCTION TO ADD CORE TRANSFORMER NAMES. Instead register them directly in the
* validTransformerNames set above.
@ -96,19 +104,17 @@ function _normalizeTransformerName(name, type) {
* @param {string} transformerType the type of the transformer being added
*/
export function _addTransformerName(name, transformerType) {
const apiName = `api.add${capitalize(
const prefix = `${consolePrefix()} api.add${capitalize(
transformerType.toLowerCase()
)}TransformerName`;
)}TransformerName`.trim();
if (name !== name.toLowerCase()) {
throw new Error(
`${apiName}: transformer name "${name}" must be lowercase.`
);
throw new Error(`${prefix}: transformer name "${name}" must be lowercase.`);
}
if (registryOpened) {
throw new Error(
`${apiName} was called when the system is no longer accepting new names to be added.` +
`${prefix} was called when the system is no longer accepting new names to be added. ` +
`Move your code to a pre-initializer that runs before "freeze-valid-transformers" to avoid this error.`
);
}
@ -127,14 +133,14 @@ export function _addTransformerName(name, transformerType) {
if (existingInfo.source === CORE_TRANSFORMER) {
// eslint-disable-next-line no-console
console.warn(
`${apiName}: transformer "${name}" matches existing core transformer "${existingInfo.name}" and shouldn't be re-registered using the the API.`
`${prefix}: transformer "${name}" matches existing core transformer "${existingInfo.name}" and shouldn't be re-registered using the the API.`
);
return;
}
// eslint-disable-next-line no-console
console.warn(
`${apiName}: transformer "${existingInfo.name}" is already registered`
`${prefix}: transformer "${existingInfo.name}" is already registered`
);
}
@ -155,14 +161,13 @@ export function _registerTransformer(
if (!transformerTypes[transformerType]) {
throw new Error(`Invalid transformer type: ${transformerType}`);
}
const apiName = `api.register${capitalize(
const prefix = `${consolePrefix()} api.register${capitalize(
transformerType.toLowerCase()
)}Transformer`;
)}Transformer`.trim();
if (!registryOpened) {
throw new Error(
`${apiName} was called while the system was still accepting new transformer names to be added.\n` +
`${prefix} was called while the system was still accepting new transformer names to be added.\n` +
`Move your code to an initializer or a pre-initializer that runs after "freeze-valid-transformers" to avoid this error.`
);
}
@ -175,14 +180,14 @@ export function _registerTransformer(
if (!transformerNameExists(normalizedTransformerName)) {
// eslint-disable-next-line no-console
console.warn(
`${apiName}: transformer "${transformerName}" is unknown and will be ignored. ` +
`${prefix}: transformer "${transformerName}" is unknown and will be ignored. ` +
"Is the name correct? Are you using the correct API for the transformer type?"
);
}
if (typeof callback !== "function") {
throw new Error(
`${apiName} requires the callback argument to be a function`
`${prefix} requires the callback argument to be a function`
);
}
@ -204,9 +209,11 @@ export function applyBehaviorTransformer(
transformerTypes.BEHAVIOR
);
const prefix = `${consolePrefix()} applyBehaviorTransformer`.trim();
if (!transformerNameExists(normalizedTransformerName)) {
throw new Error(
`applyBehaviorTransformer: transformer name "${transformerName}" does not exist. ` +
`${prefix}: transformer name "${transformerName}" does not exist. ` +
"Was the transformer name properly added? Is the transformer name correct? Is the type equals BEHAVIOR? " +
"applyBehaviorTransformer can only be used with BEHAVIOR transformers."
);
@ -214,7 +221,7 @@ export function applyBehaviorTransformer(
if (typeof defaultCallback !== "function") {
throw new Error(
`applyBehaviorTransformer requires the callback argument to be a function`
`${prefix} requires the callback argument to be a function`
);
}
@ -222,7 +229,7 @@ export function applyBehaviorTransformer(
typeof (context ?? undefined) !== "undefined" &&
!(typeof context === "object" && context.constructor === Object)
) {
throw `applyBehaviorTransformer("${transformerName}", ...): context must be a simple JS object or nullish.`;
throw `${prefix}("${transformerName}", ...): context must be a simple JS object or nullish.`;
}
const transformers = transformersRegistry.get(normalizedTransformerName);
@ -244,7 +251,28 @@ export function applyBehaviorTransformer(
return;
}
return currentCallback({ context: appliedContext, next: nextCallback });
// do not surround the default implementation in the try ... catch block
if (currentCallback === defaultCallback) {
return currentCallback({ context: appliedContext });
}
try {
return currentCallback({ context: appliedContext, next: nextCallback });
} catch (error) {
document.dispatchEvent(
new CustomEvent("discourse-error", {
detail: { messageKey: "broken_transformer_alert", error },
})
);
if (isTesting() && !skipApplyExceptionOnTests) {
throw error;
}
// if the current callback failed keep processing the callback queue
// hopefully the application won't be left in a broken state
return nextCallback();
}
}
return nextCallback();
@ -265,9 +293,11 @@ export function applyValueTransformer(transformerName, defaultValue, context) {
transformerTypes.VALUE
);
const prefix = `${consolePrefix()} applyValueTransformer`.trim();
if (!transformerNameExists(normalizedTransformerName)) {
throw new Error(
`applyValueTransformer: transformer name "${transformerName}" does not exist. ` +
`${prefix}: transformer name "${transformerName}" does not exist. ` +
"Was the transformer name properly added? Is the transformer name correct? Is the type equals VALUE? " +
"applyValueTransformer can only be used with VALUE transformers."
);
@ -278,7 +308,7 @@ export function applyValueTransformer(transformerName, defaultValue, context) {
!(typeof context === "object" && context.constructor === Object)
) {
throw (
`applyValueTransformer("${transformerName}", ...): context must be a simple JS object or nullish.\n` +
`${prefix}("${transformerName}", ...): context must be a simple JS object or nullish.\n` +
"Avoid passing complex objects in the context, like for example, component instances or objects that carry " +
"mutable state directly. This can induce users to registry transformers with callbacks causing side effects " +
"and mutating the context directly. Inevitably, this leads to fragile integrations."
@ -296,7 +326,20 @@ export function applyValueTransformer(transformerName, defaultValue, context) {
const transformerPoolSize = transformers.length;
for (let i = 0; i < transformerPoolSize; i++) {
const valueCallback = transformers[i];
newValue = valueCallback({ value: newValue, context });
try {
newValue = valueCallback({ value: newValue, context });
} catch (error) {
document.dispatchEvent(
new CustomEvent("discourse-error", {
detail: { messageKey: "broken_transformer_alert", error },
})
);
if (isTesting() && !skipApplyExceptionOnTests) {
throw error;
}
}
}
return newValue;
@ -410,6 +453,7 @@ export function resetTransformers() {
clearPluginTransformers();
transformersRegistry.clear();
skipApplyExceptionOnTests = false;
}
/**
@ -420,3 +464,14 @@ function clearPluginTransformers() {
[...validTransformerNames].filter(([, type]) => type === CORE_TRANSFORMER)
);
}
/**
* Disables throwing the exception when applying the transformers in a test environment
*
* It's only to test if the exception in handled properly
*
* USE ONLY FOR TESTING PURPOSES.
*/
export function disableThrowingApplyExceptionOnTests() {
skipApplyExceptionOnTests = true;
}

View File

@ -8,6 +8,7 @@ import {
acceptTransformerRegistrations,
applyBehaviorTransformer,
applyValueTransformer,
disableThrowingApplyExceptionOnTests,
transformerTypes,
transformerWasAdded,
} from "discourse/lib/transformer";
@ -171,6 +172,8 @@ module("Unit | Utility | transformers", function (hooks) {
module("applyValueTransformer", function (innerHooks) {
innerHooks.beforeEach(function () {
this.documentDispatchEventStub = sinon.stub(document, "dispatchEvent");
acceptNewTransformerNames();
withPluginApi("1.34.0", (api) => {
@ -181,6 +184,10 @@ module("Unit | Utility | transformers", function (hooks) {
acceptTransformerRegistrations();
});
innerHooks.afterEach(function () {
this.documentDispatchEventStub.restore();
});
test("raises an exception if the transformer name does not exist", function (assert) {
assert.throws(
() => applyValueTransformer("whatever", "foo"),
@ -347,6 +354,83 @@ module("Unit | Utility | transformers", function (hooks) {
);
});
test("exceptions are handled when applying the transformer", function (assert) {
class Testable {
#value;
constructor(value) {
this.#value = value;
}
get value1() {
return applyValueTransformer("test-value1-transformer", this.#value);
}
get value2() {
return applyValueTransformer("test-value2-transformer", this.#value);
}
}
const testObject1 = new Testable(1);
const testObject2 = new Testable(2);
withPluginApi("1.34.0", (api) => {
api.registerValueTransformer("test-value1-transformer", () => {
throw new Error("sabotaged");
});
});
assert.throws(
function () {
testObject1.value1;
},
function (error) {
return error.message === "sabotaged";
},
"by default it throws an exception on tests when the transformer registered has an error"
);
disableThrowingApplyExceptionOnTests();
assert.deepEqual(
[testObject1.value1, testObject2.value1],
[1, 2],
"it catches the exception and returns the default value when the only transformer registered has an error"
);
assert.strictEqual(
this.documentDispatchEventStub.calledWith(
sinon.match
.instanceOf(CustomEvent)
.and(sinon.match.has("type", "discourse-error"))
.and(
sinon.match.has(
"detail",
sinon.match({
messageKey: "broken_transformer_alert",
error: sinon.match
.instanceOf(Error)
.and(sinon.match.has("message", "sabotaged")),
})
)
)
),
true,
"it dispatches an event to display a message do admins when an exception is caught in a transformer"
);
withPluginApi("1.34.0", (api) => {
api.registerValueTransformer("test-value1-transformer", () => {
return 0;
});
});
assert.deepEqual(
[testObject1.value1, testObject2.value1],
[0, 0],
"it catches the exception and and keeps processing the queue when there are others transformers registered"
);
});
test("the transformer callback can receive an optional context object", function (assert) {
let expectedContext = null;
@ -431,7 +515,7 @@ module("Unit | Utility | transformers", function (hooks) {
assert.throws(
() =>
withPluginApi("1.34.0", (api) => {
withPluginApi("1.35.0", (api) => {
api.addBehaviorTransformerName("whatever");
}),
/addBehaviorTransformerName was called when the system is no longer accepting new names to be added/
@ -441,7 +525,7 @@ module("Unit | Utility | transformers", function (hooks) {
test("warns if name is already registered", function (assert) {
acceptNewTransformerNames();
withPluginApi("1.34.0", (api) => {
withPluginApi("1.35.0", (api) => {
api.addBehaviorTransformerName("home-logo-href"); // existing core transformer
// testing warning about core transformers
@ -476,7 +560,7 @@ module("Unit | Utility | transformers", function (hooks) {
test("adds a new transformer name", function (assert) {
acceptNewTransformerNames();
withPluginApi("1.34.0", (api) => {
withPluginApi("1.35.0", (api) => {
assert.strictEqual(
transformerWasAdded(
"a-new-plugin-transformer",
@ -512,7 +596,7 @@ module("Unit | Utility | transformers", function (hooks) {
assert.throws(
() =>
withPluginApi("1.34.0", (api) => {
withPluginApi("1.35.0", (api) => {
api.registerBehaviorTransformer("whatever", () => "foo"); // the name doesn't really matter at this point
}),
/was called while the system was still accepting new transformer names/
@ -520,7 +604,7 @@ module("Unit | Utility | transformers", function (hooks) {
});
test("warns if transformer is unknown", function (assert) {
withPluginApi("1.34.0", (api) => {
withPluginApi("1.35.0", (api) => {
api.registerBehaviorTransformer("whatever", () => "foo");
// testing warning about core transformers
@ -536,7 +620,7 @@ module("Unit | Utility | transformers", function (hooks) {
test("raises an exception if the callback parameter is not a function", function (assert) {
assert.throws(
() =>
withPluginApi("1.34.0", (api) => {
withPluginApi("1.35.0", (api) => {
api.registerBehaviorTransformer("whatever", "foo");
}),
/api.registerBehaviorTransformer requires the callback argument to be a function/
@ -546,7 +630,7 @@ module("Unit | Utility | transformers", function (hooks) {
test("registering a new transformer works", function (assert) {
acceptNewTransformerNames();
withPluginApi("1.34.0", (api) => {
withPluginApi("1.35.0", (api) => {
api.addBehaviorTransformerName("test-transformer");
acceptTransformerRegistrations();
@ -586,9 +670,11 @@ module("Unit | Utility | transformers", function (hooks) {
module("applyBehaviorTransformer", function (innerHooks) {
innerHooks.beforeEach(function () {
this.documentDispatchEventStub = sinon.stub(document, "dispatchEvent");
acceptNewTransformerNames();
withPluginApi("1.34.0", (api) => {
withPluginApi("1.35.0", (api) => {
api.addBehaviorTransformerName("test-behavior1-transformer");
api.addBehaviorTransformerName("test-behavior2-transformer");
});
@ -596,6 +682,10 @@ module("Unit | Utility | transformers", function (hooks) {
acceptTransformerRegistrations();
});
innerHooks.afterEach(function () {
this.documentDispatchEventStub.restore();
});
test("raises an exception if the transformer name does not exist", function (assert) {
assert.throws(
() => applyBehaviorTransformer("whatever", "foo"),
@ -783,7 +873,7 @@ module("Unit | Utility | transformers", function (hooks) {
"the default behavior doubles the value"
);
withPluginApi("1.34.0", (api) => {
withPluginApi("1.35.0", (api) => {
api.registerBehaviorTransformer(
"test-behavior1-transformer",
({ context }) => {
@ -852,7 +942,7 @@ module("Unit | Utility | transformers", function (hooks) {
const testObject = new Testable();
assert.deepEqual(testObject.value, null, "initially the value is null");
withPluginApi("1.34.0", (api) => {
withPluginApi("1.35.0", (api) => {
api.registerBehaviorTransformer(
"test-behavior1-transformer",
({ context, next }) => {
@ -924,7 +1014,7 @@ module("Unit | Utility | transformers", function (hooks) {
"the value was changed after the async behavior"
);
withPluginApi("1.34.0", (api) => {
withPluginApi("1.35.0", (api) => {
api.registerBehaviorTransformer(
"test-behavior1-transformer",
async ({ context, next }) => {
@ -945,11 +1035,114 @@ module("Unit | Utility | transformers", function (hooks) {
);
});
test("exceptions are handled when applying the transformer", function (assert) {
class Testable {
#value;
constructor(value) {
this.#value = value;
}
get value() {
return this.#value;
}
multiplyValue() {
applyBehaviorTransformer(
"test-behavior1-transformer",
() => {
this.#value *= 2;
},
{ value: this.#value, setValue: (v) => (this.#value = v) }
);
}
incValue() {
applyBehaviorTransformer(
"test-behavior2-transformer",
() => {
this.#value += 1;
},
{ value: this.#value, setValue: (v) => (this.#value = v) }
);
}
}
const testObject1 = new Testable(1);
const testObject2 = new Testable(2);
withPluginApi("1.35.0", (api) => {
api.registerBehaviorTransformer("test-behavior1-transformer", () => {
throw new Error("sabotaged");
});
});
assert.throws(
function () {
testObject1.multiplyValue();
},
function (error) {
return error.message === "sabotaged";
},
"by default it throws an exception on tests when the transformer registered has an error"
);
disableThrowingApplyExceptionOnTests();
testObject1.multiplyValue();
testObject2.multiplyValue();
assert.deepEqual(
[testObject1.value, testObject2.value],
[2, 4],
"it catches the exception and follows the default behavior when the only transformer registered has an error"
);
assert.strictEqual(
this.documentDispatchEventStub.calledWith(
sinon.match
.instanceOf(CustomEvent)
.and(sinon.match.has("type", "discourse-error"))
.and(
sinon.match.has(
"detail",
sinon.match({
messageKey: "broken_transformer_alert",
error: sinon.match
.instanceOf(Error)
.and(sinon.match.has("message", "sabotaged")),
})
)
)
),
true,
"it dispatches an event to display a message do admins when an exception is caught in a transformer"
);
withPluginApi("1.35.0", (api) => {
api.registerBehaviorTransformer(
"test-behavior1-transformer",
({ context }) => {
context.setValue(0);
}
);
});
testObject1.multiplyValue();
testObject2.multiplyValue();
assert.deepEqual(
[testObject1.value, testObject2.value],
[0, 0],
"it catches the exception and and keeps processing the queue when there are others transformers registered"
);
});
test("the transformer callback can receive an optional context object", function (assert) {
let behavior = null;
let expectedContext = null;
withPluginApi("1.34.0", (api) => {
withPluginApi("1.35.0", (api) => {
api.registerBehaviorTransformer(
"test-behavior1-transformer",
({ context }) => {
@ -1011,7 +1204,7 @@ module("Unit | Utility | transformers", function (hooks) {
`initially buildValue value only generates "!"`
);
withPluginApi("1.34.0", (api) => {
withPluginApi("1.35.0", (api) => {
api.registerBehaviorTransformer(
"test-behavior1-transformer",
({ context, next }) => {
@ -1075,7 +1268,7 @@ module("Unit | Utility | transformers", function (hooks) {
`initially buildValue value only generates "!"`
);
withPluginApi("1.34.0", (api) => {
withPluginApi("1.35.0", (api) => {
api.registerBehaviorTransformer(
"test-behavior1-transformer",
({ context }) => {
@ -1134,7 +1327,7 @@ module("Unit | Utility | transformers", function (hooks) {
`initially buildValue value only generates "!"`
);
withPluginApi("1.34.0", (api) => {
withPluginApi("1.35.0", (api) => {
api.registerBehaviorTransformer(
"test-behavior1-transformer",
({ context, next }) => {
@ -1201,7 +1394,7 @@ module("Unit | Utility | transformers", function (hooks) {
`initially buildValue value only generates "!"`
);
withPluginApi("1.34.0", (api) => {
withPluginApi("1.35.0", (api) => {
api.registerBehaviorTransformer(
"test-behavior1-transformer",
({ next, context }) => {

View File

@ -226,6 +226,7 @@ en:
broken_page_change_alert: "An onPageChange handler raised an error. Check the browser developer tools for more information."
broken_plugin_alert: "Caused by plugin '%{name}'"
broken_transformer_alert: "There was an error. Your site may not work properly."
critical_deprecation:
notice: "<b>[Admin Notice]</b> One of your themes or plugins needs updating for compatibility with upcoming Discourse core changes."