HDDS-14770. [Website] Integrate eslint and run it in github actions #359
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,170 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
|
|
||
| import js from "@eslint/js"; | ||
| import globals from "globals"; | ||
| import pluginReact from "eslint-plugin-react"; | ||
| import css from "@eslint/css"; | ||
| import { defineConfig } from "eslint/config"; | ||
| import pluginDocusaurus from "@docusaurus/eslint-plugin"; | ||
| import pluginUnusedImports from "eslint-plugin-unused-imports"; | ||
| import pluginImport from "eslint-plugin-import"; | ||
|
|
||
| const apacheLicenseRule = { | ||
| meta: { | ||
| type: "problem", | ||
| docs: { | ||
| description: "Enforce Apache License header in source files", | ||
| }, | ||
| fixable: "code", | ||
| messages: { | ||
| missingHeader: "Missing Apache License header at the top of the file", | ||
| }, | ||
| }, | ||
| create(context) { | ||
| const REQUIRED_HEADER = `/* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */`; | ||
|
|
||
| return { | ||
| Program(node) { | ||
| const sourceCode = context.sourceCode || context.getSourceCode(); | ||
| const text = sourceCode.getText(); | ||
|
|
||
| if (!text.startsWith(REQUIRED_HEADER)) { | ||
| context.report({ | ||
| node, | ||
| messageId: "missingHeader", | ||
| fix(fixer) { | ||
| return fixer.insertTextBefore(node, REQUIRED_HEADER + "\n\n"); | ||
| }, | ||
| }); | ||
| } | ||
| }, | ||
| }; | ||
| }, | ||
| }; | ||
|
|
||
| export default defineConfig([ | ||
| // General | ||
| { | ||
| ignores: [ | ||
| "static/**", | ||
| "node_modules/**", | ||
| "build/**", | ||
| "dist/**", | ||
| ".docusaurus/**", | ||
| ".github/**", | ||
| ], | ||
| }, | ||
|
|
||
| // JS | ||
| { | ||
| files: ["**/*.{js,mjs,cjs,jsx}"], | ||
| ...js.configs.recommended, | ||
| languageOptions: { globals: { ...globals.browser, ...globals.node } }, | ||
|
Contributor
There was a problem hiding this comment. Cursor made this suggestion, although I don't know enough about javascript to really understand the implications: Medium: browser and node globals are both enabled for all JS files, which can hide environment mistakes.
Contributor
Author
There was a problem hiding this comment. Well, it might be valid, but not for this project. Docusaurus has SSR/browser duality by design, the mixed globals are intentional and splitting them could produce spurious lint errors in components that legitimately need awareness of both environments. We cannot clearly separate the use of node/browser globals by the file extension/location. You may want to check this out https://docusaurus.io/docs/advanced/ssg |
||
| plugins: { | ||
| "unused-imports": pluginUnusedImports, | ||
| import: pluginImport, | ||
| }, | ||
| rules: { | ||
|
Contributor
There was a problem hiding this comment. Do we want to add a rule to enforce import order?
Contributor
Author
There was a problem hiding this comment. we can have it like this "import/order": [
"error",
{
groups: [
"builtin", // Node built-ins (path, fs, etc.)
"external", // npm packages (react, @docusaurus/*, etc.)
"internal", // paths configured as internal (e.g. @site/*)
["parent", "sibling", "index"], // relative imports
],
alphabetize: { order: "asc", caseInsensitive: true },
},
], |
||
| "unused-imports/no-unused-imports": "error", | ||
| // unused vars (but ignore underscore-prefixed) | ||
| "unused-imports/no-unused-vars": [ | ||
| "warn", | ||
|
Contributor
There was a problem hiding this comment. I don't think warning are useful, people will only check if the command passes or fails. If we care about something we should make it an error. Unused variables should fail linting I think. |
||
| { | ||
| vars: "all", | ||
| varsIgnorePattern: "^_", | ||
| args: "after-used", | ||
| argsIgnorePattern: "^_", | ||
| }, | ||
| ], | ||
| "import/no-duplicates": "error", | ||
| "no-unused-vars": "off", | ||
| }, | ||
| }, | ||
|
|
||
| // React | ||
| { | ||
| files: ["**/*.{js,jsx}"], | ||
| ...pluginReact.configs.flat.recommended, | ||
| settings: { | ||
| react: { version: "detect" }, | ||
| }, | ||
| rules: { | ||
| "react/prop-types": "off", | ||
| "react/react-in-jsx-scope": "off", | ||
| "react/jsx-uses-vars": "error", | ||
| }, | ||
| }, | ||
|
|
||
| // CSS | ||
| { | ||
| files: ["**/*.css"], | ||
| plugins: { css }, | ||
| language: "css/css", | ||
| extends: ["css/recommended"], | ||
| rules: { | ||
| "css/no-invalid-properties": "off", | ||
| "css/no-important": "off", | ||
| "css/use-baseline": "off", | ||
| }, | ||
| }, | ||
|
|
||
| // Docusaurus | ||
| { | ||
| files: ["**/*.{js,mjs,cjs,jsx}"], | ||
| plugins: { "@docusaurus": pluginDocusaurus }, | ||
| rules: { | ||
| "@docusaurus/no-html-links": "error", | ||
| "@docusaurus/prefer-docusaurus-heading": "error", | ||
| "@docusaurus/string-literal-i18n-messages": "error", | ||
| // for i18n | ||
| // "@docusaurus/no-untranslated-text": ["warn", { ignoredStrings: [] }] | ||
|
Contributor
There was a problem hiding this comment. Does this rule fail even if i18n is disabled? If it passes trivially in this case then we can probably reduce this to just use the default recommended config instead of specifying rules individually.
Contributor
Author
There was a problem hiding this comment. the i18n rule should pass if i18n is not enabled. there are just 4 rules in total, and we're basically overriding them. Default: '@docusaurus/string-literal-i18n-messages': 'error', // for us it's also error
'@docusaurus/no-html-links': 'warn', // we use error
'@docusaurus/prefer-docusaurus-heading': 'warn', // we also use errorThis one "@docusaurus/no-untranslated-text": ["warn", { ignoredStrings: [] }]is commented out right now, since we plan want to start using i18n later, as far as I know |
||
| }, | ||
| }, | ||
|
|
||
| // Custom rule to enforce Apache License header | ||
| { | ||
| files: ["**/*.{js,mjs,cjs,jsx}"], | ||
|
Comment on lines
+158
to
+160
Contributor
There was a problem hiding this comment. I don't think we need this. We already have a license header check using skywalking-eyes for all files in CI which is more robust than this strict string constant header check. This does run locally on I think we can remove this to keep our linting config simple and reduce duplication/conflicts with skywalking-eyes. In a follow-up change it would be good to have skywalking-eyes run locally on lint too, but that would also require a separate install outside of node.js.
Contributor
Author
There was a problem hiding this comment. Sure, I can remove this |
||
| plugins: { | ||
| custom: { | ||
| rules: { "apache-license": apacheLicenseRule }, | ||
| }, | ||
| }, | ||
| rules: { | ||
| "custom/apache-license": "error", | ||
| }, | ||
| }, | ||
| ]); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a dist directory in the repo. Does something generate this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
distaccidentally slipped in, I can remove it. Usually many modern projects usedistinstead ofbuild, but we don't