diff --git a/.github/actions/setup/action.yml b/.github/actions/setup/action.yml deleted file mode 100644 index 373a9f9..0000000 --- a/.github/actions/setup/action.yml +++ /dev/null @@ -1,15 +0,0 @@ -name: Setup project -description: Sets up the repo code, Node.js, and npm dependencies - -runs: - using: composite - steps: - - name: Set up Node.js - uses: actions/setup-node@v2 - with: - node-version: '16.x' - cache: npm - registry-url: https://registry.npmjs.org - - name: Install npm dependencies - run: npm ci - shell: bash diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml new file mode 100644 index 0000000..c1b229d --- /dev/null +++ b/.github/workflows/publish.yml @@ -0,0 +1,24 @@ +name: Publish + +on: + release: + types: [created] + +jobs: + publish-npm: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v3 + - uses: actions/setup-node@v3 + with: + node-version: 14 + registry-url: https://registry.npmjs.org/ + cache: npm + - run: npm ci + - run: npm test + - run: npm version ${TAG_NAME} --git-tag-version=false + env: + TAG_NAME: ${{ github.event.release.tag_name }} + - run: npm whoami; npm --ignore-scripts publish + env: + NODE_AUTH_TOKEN: ${{secrets.npm_token}} diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml deleted file mode 100644 index 11a2fe6..0000000 --- a/.github/workflows/release.yml +++ /dev/null @@ -1,75 +0,0 @@ -name: Release and publish - -# Inspired by https://github.com/MylesBorins/node-osc/blob/959b9c83972a67390a351d333b23db3972ca7b46/.github/workflows/bump-version.yml and -# https://github.com/MylesBorins/node-osc/blob/74b563c83736a04c4a37acbff9d7bb1f01a00f57/.github/workflows/create-release.yml - -on: - workflow_dispatch: - inputs: - version: - description: Semver descriptor for new version ("major", "minor", or "patch") - required: true - -jobs: - bump-version: - name: Bump package version - runs-on: ubuntu-latest - outputs: - new-tag: ${{ steps.new-tag.outputs.new-tag }} - steps: - - name: Checkout ref - uses: actions/checkout@v2 - - name: Preparation - uses: ./.github/actions/setup - - name: Perform last-minute tests - run: npm test - - name: Configure Git - run: | - git config user.name "GitHub Actions" - git config user.email "actions@github.com" - - name: Bump package version - run: npm version ${{ github.event.inputs.version }} - - name: Push back to GitHub - run: git push origin main --follow-tags - - name: Set output to new version - id: new-tag - run: | - version=$(jq -r .version < package.json) - echo "::set-output name=new-tag::v$version" - create-release: - name: Create GitHub release - runs-on: ubuntu-latest - needs: bump-version - steps: - - name: Checkout ref - uses: actions/checkout@v2 - with: - ref: ${{ needs.bump-version.outputs.new-tag }} - - name: Preparation - uses: ./.github/actions/setup - - name: Create release - uses: actions/github-script@v5 - with: - github-token: ${{ secrets.GITHUB_TOKEN }} - script: | - await github.request(`POST /repos/${{ github.repository }}/releases`, { - tag_name: "${{ needs.bump-version.outputs.new-tag }}", - generate_release_notes: true - }) - publish: - name: Publish to npm - runs-on: ubuntu-latest - needs: [bump-version, create-release] - steps: - - name: Checkout ref - uses: actions/checkout@v2 - with: - ref: ${{ needs.bump-version.outputs.new-tag }} - - name: Preparation - uses: ./.github/actions/setup - - name: Build package - run: npm run build --if-present - - name: Publish - run: npm publish --access public - env: - NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }} diff --git a/README.md b/README.md index ba82e4c..2c2cfb8 100644 --- a/README.md +++ b/README.md @@ -32,6 +32,7 @@ JSON ESLint config example: - [File Name Matches Element](./docs/rules/file-name-matches-element.md) - [No Constructor](./docs/rules/no-constructor.md) - [No Customized Built in Elements](./docs/rules/no-customized-built-in-elements.md) +- [No DOM Traversal in Attributechangedcallback](./docs/rules/no-dom-traversal-in-attributechangedcallback.md) - [No DOM Traversal in Connectedcallback](./docs/rules/no-dom-traversal-in-connectedcallback.md) - [No Exports with Element](./docs/rules/no-exports-with-element.md) - [No Method Prefixed with on](./docs/rules/no-method-prefixed-with-on.md) diff --git a/docs/rules/no-dom-traversal-in-attributechangedcallback.md b/docs/rules/no-dom-traversal-in-attributechangedcallback.md new file mode 100644 index 0000000..ecbb888 --- /dev/null +++ b/docs/rules/no-dom-traversal-in-attributechangedcallback.md @@ -0,0 +1,100 @@ +# No DOM Traversal in Attributechangedcallback + +The intent of the `attributeChangedCallback` is to initialise state based on attribute values, and to observe and alter state when those values change. Traversing the DOM during the `attributeChangedCallback` phase is error-prone, because: + +- it can fire before `connectedCallback`, meaning any initialisation inside of `connectedCallback` has not yet occurred. +- it can fire before `this.isConnected` is `true`, meaning the node has been created but is not yet appended to the DOM. For example `this.ownerDocument` or `this.parent` will be null, and `this.querySelector()` will return `null` for any value. + +To give a concrete example of this, there is a common pattern when constructing DOM in JS to create the element, set its attributes and append to the DOM, in that order. In code this might look like: + +```js +const el = document.createElement('foo-bar') +el.setAttribute('baz', 'bing') +// attributeChangedCallback('baz', null, 'bing') is fired! +document.body.append(el) +// connectedCallback() is fired! +``` + +This is also true for document parsing in general: attributes that are in the HTML during parse time will call `attributeChangedCallback` before `connectedCallback`. This could be represented in code like: + +```js +document.body.innerHTML = '' +// attributeChangedCallback('baz', null, 'bing') is fired! +// connectedCallback() is fired! +``` + +Guarding against `null` properties, or returning early for `isConnected === false` is not good enough because there is high risk that attribute changes won't be properly propagated and state can fall out of sync. Guarding against these means adding duplicate code in other lifecycle callbacks such as `connectedCallback` to ensure this state does not fall out of sync. It is instead preferable to move such DOM traversals away from `attributeChangedCallback`, using one of the following: + +- move traversals to `connectedCallback` +- dispatch events from `attributeChangedCallback`, binding event listeners on the element itself within `connectedCallback` +- defer DOM traversals to just-in-time lookup using methods or getters. + +All of these patterns still mean state initialisation should be done in the `connectedCallback`. Do not rely on `attributeChangedCallback` for state initialisation. + +## Rule Details + +This rule disallows using DOM traversal APIs within the `attributeChangedCallback`. + +👎 Examples of **incorrect** code for this rule: + +```js +class FooBarElement extends HTMLElement { + attributeChangedCallback(name, _, value) { + if (name === 'aria-owns') { + // This has not been guarded against `this.isConnected` and so + // `ownerDocument` is null. + this.mine = this.ownerDocument.getElementById(value) + } + } +} +``` + +```js +class FooBarElement extends HTMLElement { + attributeChangedCallback(name, _, value) { + if (name === 'data-text') { + // This has not been guarded against `this.isConnected` and so + // `ownerDocument` is null. + this.querySelector('span').textContent = value + } + } +} +``` + +👍 Examples of **correct** code for this rule: + +```js +class FooBarElement extends HTMLElement { + get mine() { + return this.ownerDocument.getElementById(this.getAttribute('aria-owns')) + } +} +``` + +```js +class FooBarElement extends HTMLElement { + attributeChangedCallback(name, _, value) { + if (this.isConnected && name === 'data-text') { + // Guarding with `isConnected` can be used here, but we also + // need to synchronise this state in the `connectedCallback` as well. + this.update() + } + } + update() { + this.querySelector('span').textContent = this.getAttribute('data-text') + } + connectedCallback() { + // This needs to happen because `attributeChangedCallback` doesn't + // _always_ update. + this.update() + } +} +``` + +## When Not To Use It + +If you are comfortable with the edge cases of DOM traversal directly in the `attributeChangedCallback` then you can disable this rule. + +## Version + +This rule was introduced in 0.0.5 diff --git a/lib/dom.js b/lib/dom.js new file mode 100644 index 0000000..6f3510b --- /dev/null +++ b/lib/dom.js @@ -0,0 +1,35 @@ +module.exports = { + properties: new Set([ + // ParentNode + 'childElementCount', + 'children', + 'firstElementChild', + 'lastElementChild', + + // Node + 'childNodes', + 'firstChild', + 'lastChild', + 'innerText', + 'innerHTML', + 'textContent' + ]), + methods: new Set([ + // Document + 'getElementById', + 'getElementsByClassName', + 'getElementsByTagName', + + // ParentNode + 'querySelector', + 'querySelectorAll', + + // Node + 'contains', + 'hasChildNodes', + 'insertBefore', + 'removeChild', + 'replaceChild' + ]), + allowedScopes: new Set(['addEventListener', 'MutationObserver']) +} diff --git a/lib/name-transforms.js b/lib/name-transforms.js index 4057abc..841ca24 100644 --- a/lib/name-transforms.js +++ b/lib/name-transforms.js @@ -20,22 +20,22 @@ module.exports = { *generateNames(prefixes, suffixes, name) { for (const prefix of prefixes) { if (name.toLowerCase().startsWith(prefix.toLowerCase())) { - const truncated = name.substr(prefix.length) + const truncated = name.slice(prefix.length) yield truncated for (const suffix of suffixes) { if (truncated.toLowerCase().endsWith(suffix.toLowerCase())) { - yield truncated.substr(0, truncated.length - suffix.length) + yield truncated.slice(0, truncated.length - suffix.length) } } } } for (const suffix of suffixes) { if (name.toLowerCase().endsWith(suffix.toLowerCase())) { - const truncated = name.substr(0, name.length - suffix.length) + const truncated = name.slice(0, name.length - suffix.length) yield truncated for (const prefix of prefixes) { if (truncated.toLowerCase().startsWith(prefix.toLowerCase())) { - yield truncated.substr(prefix.length) + yield truncated.slice(prefix.length) } } } diff --git a/lib/rules.js b/lib/rules.js index d8b7043..13cd834 100644 --- a/lib/rules.js +++ b/lib/rules.js @@ -5,6 +5,7 @@ module.exports = { 'file-name-matches-element': require('./rules/file-name-matches-element'), 'no-constructor': require('./rules/no-constructor'), 'no-customized-built-in-elements': require('./rules/no-customized-built-in-elements'), + 'no-dom-traversal-in-attributechangedcallback': require('./rules/no-dom-traversal-in-attributechangedcallback'), 'no-dom-traversal-in-connectedcallback': require('./rules/no-dom-traversal-in-connectedcallback'), 'no-exports-with-element': require('./rules/no-exports-with-element'), 'no-method-prefixed-with-on': require('./rules/no-method-prefixed-with-on'), diff --git a/lib/rules/no-dom-traversal-in-attributechangedcallback.js b/lib/rules/no-dom-traversal-in-attributechangedcallback.js new file mode 100644 index 0000000..422c824 --- /dev/null +++ b/lib/rules/no-dom-traversal-in-attributechangedcallback.js @@ -0,0 +1,24 @@ +const s = require('../custom-selectors') +const dom = require('../dom') +module.exports = { + meta: { + type: 'suggestion', + docs: {description: '', url: require('../url')(module)} + }, + schema: [], + create(context) { + return { + [`${s.HTMLElementClass} MethodDefinition[key.name="attributeChangedCallback"] MemberExpression`](node) { + if (node.parent.type === 'AssignmentExpression') return + const name = node.property.name + + if (node.parent.type === 'CallExpression' && node.parent.callee === node && dom.methods.has(name)) { + context.report(node.parent, `DOM traversal using .${name} inside attributeChangedCallback() is error prone.`) + } + if (!node.property || dom.properties.has(name)) { + context.report(node, `DOM traversal using .${name} inside attributeChangedCallback() is error prone.`) + } + } + } + } +} diff --git a/lib/rules/no-dom-traversal-in-connectedcallback.js b/lib/rules/no-dom-traversal-in-connectedcallback.js index 6e9e0ab..1142f54 100644 --- a/lib/rules/no-dom-traversal-in-connectedcallback.js +++ b/lib/rules/no-dom-traversal-in-connectedcallback.js @@ -1,34 +1,5 @@ const s = require('../custom-selectors') -const properties = new Set([ - // ParentNode - 'childElementCount', - 'children', - 'firstElementChild', - 'lastElementChild', - - // Node - 'childNodes', - 'firstChild', - 'lastChild', - 'innerText', - 'innerHTML', - 'textContent' -]) -const methods = new Set([ - // ParentNode - 'querySelector', - 'querySelectorAll', - - // Node - 'contains', - 'hasChildNodes', - 'insertBefore', - 'removeChild', - 'replaceChild' -]) - -const allowedScopes = new Set(['addEventListener', 'MutationObserver']) - +const dom = require('../dom') module.exports = { meta: { type: 'suggestion', @@ -43,14 +14,14 @@ module.exports = { const scope = context.getScope() if (scope.type === 'function') { const functionScopeName = scope.block.parent.callee?.name || scope.block.parent.callee?.property?.name - if (allowedScopes.has(functionScopeName)) return + if (dom.allowedScopes.has(functionScopeName)) return } const name = node.property.name - if (node.parent.type === 'CallExpression' && node.parent.callee === node && methods.has(name)) { + if (node.parent.type === 'CallExpression' && node.parent.callee === node && dom.methods.has(name)) { context.report(node.parent, `DOM traversal using .${name} inside connectedCallback() is error prone.`) } - if (!node.property || properties.has(name)) { + if (!node.property || dom.properties.has(name)) { context.report(node, `DOM traversal using .${name} inside connectedCallback() is error prone.`) } } diff --git a/package-lock.json b/package-lock.json index 896e42d..6865b62 100644 --- a/package-lock.json +++ b/package-lock.json @@ -2291,9 +2291,9 @@ } }, "node_modules/minimist": { - "version": "1.2.5", - "resolved": "https://registry.npmjs.org/minimist/-/minimist-1.2.5.tgz", - "integrity": "sha512-FM9nNUYrRBAELZQT3xeZQ7fmMOBg6nWNmJKTcgsJeaLstP/UODVpGsr5OhXhhXg6f+qtJ8uiZ+PUxkDWcgIXLw==", + "version": "1.2.6", + "resolved": "https://registry.npmjs.org/minimist/-/minimist-1.2.6.tgz", + "integrity": "sha512-Jsjnk4bw3YJqYzbdyBiNsPWHPfO++UGG749Cxs6peCu5Xg4nrena6OVxOYxrQTqww0Jmwt+Ref8rggumkTLz9Q==", "dev": true }, "node_modules/mocha": { @@ -4891,9 +4891,9 @@ } }, "minimist": { - "version": "1.2.5", - "resolved": "https://registry.npmjs.org/minimist/-/minimist-1.2.5.tgz", - "integrity": "sha512-FM9nNUYrRBAELZQT3xeZQ7fmMOBg6nWNmJKTcgsJeaLstP/UODVpGsr5OhXhhXg6f+qtJ8uiZ+PUxkDWcgIXLw==", + "version": "1.2.6", + "resolved": "https://registry.npmjs.org/minimist/-/minimist-1.2.6.tgz", + "integrity": "sha512-Jsjnk4bw3YJqYzbdyBiNsPWHPfO++UGG749Cxs6peCu5Xg4nrena6OVxOYxrQTqww0Jmwt+Ref8rggumkTLz9Q==", "dev": true }, "mocha": { diff --git a/test/check-rules.js b/test/check-rules.js index d8865b9..81f2aeb 100644 --- a/test/check-rules.js +++ b/test/check-rules.js @@ -18,7 +18,7 @@ function rulesFromDir(dir) { function makeTitle(name) { return name .replace(/-/g, ' ') - .replace(/\w\S*/g, x => x.charAt(0).toUpperCase() + x.substr(1)) + .replace(/\w\S*/g, x => x.charAt(0).toUpperCase() + x.slice(1)) .replace(/\b(The|An?|And|To|In|On|With)\b/g, x => x.toLowerCase()) .replace(/\b(Dom)\b/g, x => x.toUpperCase()) } diff --git a/test/no-dom-traversal-in-attributechangedcallback.js b/test/no-dom-traversal-in-attributechangedcallback.js new file mode 100644 index 0000000..6a75c56 --- /dev/null +++ b/test/no-dom-traversal-in-attributechangedcallback.js @@ -0,0 +1,121 @@ +const rule = require('../lib/rules/no-dom-traversal-in-attributechangedcallback') +const RuleTester = require('eslint').RuleTester + +const ruleTester = new RuleTester({env: {es2020: true}}) + +ruleTester.run('no-dom-traversal-in-attributeChangedCallback', rule, { + valid: [ + {code: 'document.querySelector("hello")'}, + {code: 'class FooBar { attributeChangedCallback() { this.querySelector("hello") } }'}, + {code: 'class FooBar extends HTMLElement { update() { this.querySelector("hello") } }'}, + {code: 'class FooBar extends HTMLElement { attributeChangedCallback() { this.innerHTML = "

foo

" } }'}, + {code: 'document.children'}, + { + code: 'class FooBar extends HTMLElement { attributeChangedCallback() { this.children = [1] } }' + } + ], + invalid: [ + { + code: 'class FooBar extends HTMLElement { attributeChangedCallback() { this.querySelector("hello") } }', + errors: [ + { + message: 'DOM traversal using .querySelector inside attributeChangedCallback() is error prone.', + type: 'CallExpression' + } + ] + }, + { + code: 'class FooBar extends HTMLElement { attributeChangedCallback() { document.querySelector("hello") } }', + errors: [ + { + message: 'DOM traversal using .querySelector inside attributeChangedCallback() is error prone.', + type: 'CallExpression' + } + ] + }, + { + code: 'class FooBar extends HTMLElement { attributeChangedCallback() { this.foo.querySelector("hello") } }', + errors: [ + { + message: 'DOM traversal using .querySelector inside attributeChangedCallback() is error prone.', + type: 'CallExpression' + } + ] + }, + { + code: 'class FooBar extends HTMLElement { attributeChangedCallback() { this.children } }', + errors: [ + { + message: 'DOM traversal using .children inside attributeChangedCallback() is error prone.', + type: 'MemberExpression' + } + ] + }, + { + code: 'class FooBar extends HTMLElement { attributeChangedCallback() { console.log(this.innerHTML) } }', + errors: [ + { + message: 'DOM traversal using .innerHTML inside attributeChangedCallback() is error prone.', + type: 'MemberExpression' + } + ] + }, + { + code: 'class FooBar extends HTMLElement { attributeChangedCallback() { console.log(this.innerText) } }', + errors: [ + { + message: 'DOM traversal using .innerText inside attributeChangedCallback() is error prone.', + type: 'MemberExpression' + } + ] + }, + { + code: 'class FooBar extends HTMLElement { attributeChangedCallback() { console.log(this.textContent) } }', + errors: [ + { + message: 'DOM traversal using .textContent inside attributeChangedCallback() is error prone.', + type: 'MemberExpression' + } + ] + }, + { + code: ` +class FooBarElement extends HTMLElement { + attributeChangedCallback() { + this.addEventListener("update", () => { + const button = this.querySelector('button') + if (button) { + button.disabled = true + } + }) + } +} +`, + errors: [ + { + message: 'DOM traversal using .querySelector inside attributeChangedCallback() is error prone.', + type: 'CallExpression' + } + ] + }, + { + code: ` +class FooBarElement extends HTMLElement { + attributeChangedCallback() { + new MutationObserver(() => { + const button = this.querySelector('button') + if (button) { + button.disabled = true + } + }).observe(this) + } +}`, + errors: [ + { + message: 'DOM traversal using .querySelector inside attributeChangedCallback() is error prone.', + type: 'CallExpression' + } + ] + } + ] +})