Skip to content

Commit

Permalink
🏗 Lint uses of core/dom/jsx (#36881)
Browse files Browse the repository at this point in the history
Closes #36679

This handles all caveats mentioned except mapped attribute names, which are handled by `local/preact-preferred-props`

Also transfers relevant rules from `local/preact` into `local/core-dom-jsx`.
  • Loading branch information
alanorozco committed Nov 11, 2021
1 parent 9ca510f commit b89aa33
Show file tree
Hide file tree
Showing 7 changed files with 180 additions and 73 deletions.
1 change: 1 addition & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ module.exports = {
'local/await-expect': 2,
'local/camelcase': 2,
'local/closure-type-primitives': 2,
'local/core-dom-jsx': 2,
'local/dict-string-keys': 2,
'local/enums': 2,
'local/get-mode-usage': 2,
Expand Down
174 changes: 174 additions & 0 deletions build-system/eslint-rules/core-dom-jsx.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,174 @@
/**
* @fileoverview Lints JSX features not supported by core/dom/jsx
*/
const astUtils = require('eslint/lib/rules/utils/ast-utils');

module.exports = function (context) {
let isCoreDomJsx = false;

/**
* @param {import('eslint').Node} node
* @param {boolean} isClass
* @return {boolean}
*/
function isValidStyleOrClassValue(node, isClass = false) {
if (node?.type === 'ConditionalExpression') {
// Both possible outcomes of a ternary must be valid
return (
isValidStyleOrClassValue(node.consequent, isClass) &&
isValidStyleOrClassValue(node.alternate, isClass)
);
} else if (node?.type === 'BinaryExpression') {
// Concatenating anything to a string is valid
if (node.operator === '+') {
return (
isValidStyleOrClassValue(node.left, isClass) ||
isValidStyleOrClassValue(node.right, isClass)
);
}
} else if (node?.type === 'LogicalExpression') {
if (node.operator === '||' || node.operator === '??') {
return (
isValidStyleOrClassValue(node.left, isClass) &&
isValidStyleOrClassValue(node.right, isClass)
);
}
// Any left falsy is okay
if (node.operator === '&&') {
return isValidStyleOrClassValue(node.right, isClass);
}
} else if (node?.type === 'CallExpression') {
// Calls to functions that return a string
const {name} = node.callee;
return name === 'String' || (isClass && name?.toLowerCase() === 'objstr');
} else if (node?.type === 'ObjectExpression') {
// Style attributes can be objects
return !isClass;
}
return node?.type === 'Literal' || node?.type === 'TemplateLiteral';
}

return {
Program() {
isCoreDomJsx = false;
},
ImportNamespaceSpecifier(node) {
if (node.parent.source.value.endsWith('core/dom/jsx')) {
isCoreDomJsx = true;
}
},
JSXFragment(node) {
if (!isCoreDomJsx) {
return;
}
context.report({
node,
message:
'Fragments are not supported. Change into an array of elements, or wrap in a root element.',
});
},
JSXOpeningElement(node) {
if (!isCoreDomJsx) {
return;
}
const {name} = node;
if (name.name === 'foreignObject') {
context.report({
node: node.name,
message: `<${node.name.name}> is not supported.`,
});
}

if (name.type === 'JSXMemberExpression') {
return context.report({
node,
message: [
'Static JSX Templates are required to use regular DOM nodes or Imported Components',
'This prevents an issue with `<json.type />` accidentally creating a <script> node.',
].join('\n\t'),
});
}

if (name.name && /^[a-z]/.test(name.name)) {
return;
}

const variable = astUtils.getVariableByName(
context.getScope(),
name.name
);

if (!variable || variable.defs.length === 0) {
return context.report({
node,
message: `Could not find ${name.name} in the lexcial scope`,
});
}

for (const def of variable.defs) {
if (def.type === 'ImportBinding' || def.type === 'FunctionName') {
continue;
}

context.report({
node,
message: [
'Static JSX Templates are required to use regular DOM nodes or Imported Components',
'This prevents an issue with `<UserProvidedType />` accidentally creating a <script> node.',
].join('\n\t'),
});
}
},
JSXSpreadAttribute(node) {
if (!isCoreDomJsx) {
return;
}

context.report({
node,
message: [
'Static JSX Templates are required to use static attribute definitions',
'This prevents an issue with spread attributes accidentally overriding a "safe" attribute with user-provided data.',
].join('\n\t'),
});
},
JSXAttribute(node) {
if (!isCoreDomJsx) {
return;
}
const {name} = node.name;
if (name === 'dangerouslySetInnerHTML') {
context.report({
node: node.name,
message: `\`<${name}>\` is not supported.`,
});
return;
}
const {value} = node;
if (!value || value.type === 'Literal') {
return;
}
if (name === 'class') {
if (!isValidStyleOrClassValue(value.expression, /* isClass */ true)) {
context.report({
node: node.value,
message: [
`The inline result of \`${name}\` must resolve to a "string", a \`template \${literal}\`, or a call to either objstr() or String().`,
`Take caution when wrapping boolean or nullish values in String(). Do \`String(foo || '')\``,
].join('\n - '),
});
}
} else if (name === 'style') {
if (!isValidStyleOrClassValue(value.expression)) {
context.report({
node: node.value,
message: [
`The inline result of \`${name}\` must resolve to an {objectExpression: ''}, a "string", a \`template \${literal}\`, or a call to String().`,
`Take caution when wrapping boolean or nullish values in String(). Do \`String(foo || '')\``,
].join('\n - '),
});
}
}
},
};
};
66 changes: 0 additions & 66 deletions build-system/eslint-rules/preact.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
'use strict';

const astUtils = require('eslint/lib/rules/utils/ast-utils');

// Enforces importing a Preact namespace specifier if using JSX
//
// Good
Expand Down Expand Up @@ -61,19 +59,16 @@ module.exports = {

let imported = false;
let warned = false;
let staticTemplate = false;

return {
Program() {
imported = false;
warned = false;
staticTemplate = false;
},

ImportNamespaceSpecifier(node) {
if (node.local.name === 'Preact') {
imported = true;
staticTemplate = node.parent.source.value !== '#preact';
}
},

Expand All @@ -84,67 +79,6 @@ module.exports = {
JSXFragment(node) {
requirePreact(node);
},

JSXSpreadAttribute(node) {
if (!staticTemplate) {
return;
}

context.report({
node,
message: [
'Static JSX Templates are required to use static attribute definitions',
'This prevents an issue with spread attributes accidentally overriding a "safe" attribute with user-provided data.',
].join('\n\t'),
});
},

JSXOpeningElement(node) {
if (!staticTemplate) {
return;
}

const {name} = node;
if (name.type === 'JSXMemberExpression') {
return context.report({
node,
message: [
'Static JSX Templates are required to use regular DOM nodes or Imported Components',
'This prevents an issue with `<json.type />` accidentally creating a <script> node.',
].join('\n\t'),
});
}

if (name.name && /^[a-z]/.test(name.name)) {
return;
}

const variable = astUtils.getVariableByName(
context.getScope(),
name.name
);

if (!variable || variable.defs.length === 0) {
return context.report({
node,
message: `Could not find ${name.name} in the lexcial scope`,
});
}

for (const def of variable.defs) {
if (def.type === 'ImportBinding' || def.type === 'FunctionName') {
continue;
}

context.report({
node,
message: [
'Static JSX Templates are required to use regular DOM nodes or Imported Components',
'This prevents an issue with `<UserProvidedType />` accidentally creating a <script> node.',
].join('\n\t'),
});
}
},
};
},
};
2 changes: 1 addition & 1 deletion extensions/amp-story/1.0/amp-story-system-layer.js
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ const HIDE_MESSAGE_TIMEOUT_MS = 1500;
*/
const renderSystemLayerElement = (element) => (
<aside class="i-amphtml-story-system-layer i-amphtml-story-system-reset">
<a class={ATTRIBUTION_CLASS} target="_blank">
<a class={String(ATTRIBUTION_CLASS)} target="_blank">
<div class="i-amphtml-story-attribution-logo-container">
<img alt="" class="i-amphtml-story-attribution-logo" />
</div>
Expand Down
5 changes: 1 addition & 4 deletions extensions/amp-story/1.0/toast.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,6 @@ import {Services} from '#service';
import {removeElement} from '#core/dom';
import {getWin} from '#core/window';

/** @private @const {string} */
const TOAST_CLASSNAME = 'i-amphtml-story-toast';

/**
* The 'alert' role assertively announces toast content to screen readers.
* @private @const {string}
Expand All @@ -31,7 +28,7 @@ export class Toast {
const win = getWin(storyEl);

const toast = (
<div class={TOAST_CLASSNAME} role={TOAST_ROLE}>
<div class="i-amphtml-story-toast" role={TOAST_ROLE}>
{childNodeOrText}
</div>
);
Expand Down
2 changes: 0 additions & 2 deletions src/core/dom/jsx.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,6 @@
* - This tag is rather obscure. You should be able to restructure your tree.
* If you absolutely need it, get in touch with `@alanorozco` to consider
* enabling support.
*
* TODO(https://go.amp.dev/issue/36679): Lint these unsupported features.
*/
import {devAssert} from '#core/assert';

Expand Down
3 changes: 3 additions & 0 deletions test/unit/core/dom/test-jsx.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import {dispatchCustomEvent} from '#core/dom/index';
import * as Preact from '#core/dom/jsx';

// We test invalid uses, so we disable the lint rule.
/* eslint-disable local/core-dom-jsx */

const {createElement} = Preact;

describes.sandboxed('#core/dom/jsx', {}, (env) => {
Expand Down

0 comments on commit b89aa33

Please sign in to comment.