Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Cleanup eslint config #562

Merged
merged 9 commits into from
Jun 21, 2021
Merged

Cleanup eslint config #562

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6d82e73
Enable CI job for pushes (temporarily?)
fregante Jun 21, 2021
0af9b64
Configuration cleanup
fregante Jun 21, 2021
c0f6fef
Resolve all errors
fregante Jun 21, 2021
5f56f9d
Drop outdated comment
fregante Jun 21, 2021
ba4b86b
Restore types.ts
fregante Jun 21, 2021
92f1055
Restore registerPolyfill.ts
fregante Jun 21, 2021
d926449
Merge branch 'main' into dx/eslint
fregante Jun 21, 2021
dab18d2
Mention `import/*` rules in the config + TODO
fregante Jun 21, 2021
83b9393
Prettier run
fregante Jun 21, 2021
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
57 changes: 40 additions & 17 deletions .eslintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
{
"root": true,
"parser": "@typescript-eslint/parser",
"plugins": [
"@typescript-eslint",
"security"
],
"plugins": ["@typescript-eslint", "security"],
"env": {
"browser": true
},
Expand All @@ -13,6 +10,9 @@
"webpack": {
"config": "./browsers/webpack.config.js"
}
},
"react": {
"version": "detect"
}
},
"extends": [
Expand All @@ -21,28 +21,51 @@
"plugin:@typescript-eslint/recommended",
"plugin:react/recommended",
"plugin:react-hooks/recommended",
"plugin:import/errors",
"plugin:import/warnings",
twschiller marked this conversation as resolved.
Show resolved Hide resolved
"plugin:import/typescript",
"plugin:security/recommended"

// TODO: Restore these after https://github.com/benmosher/eslint-plugin-import/issues/1931
// "plugin:import/errors",
// "plugin:import/warnings",
],
"rules": {
"@typescript-eslint/no-empty-function": "off",
"react/prop-types": "off",
"@typescript-eslint/ban-types": ["warn"],
"@typescript-eslint/no-explicit-any": ["warn"],
"@typescript-eslint/ban-ts-comment": ["error", {
"ts-ignore": "allow-with-description",
"minimumDescriptionLength": 3
}]
"@typescript-eslint/ban-ts-comment": [
"error",
{
"ts-ignore": "allow-with-description",
"minimumDescriptionLength": 3
}
]
},
"ignorePatterns": [
"browsers/chrome/webpack/**",
"browsers/firefox/webpack/**",
"browsers/webpack.config.js",
"scripts/webpack.scripts.js",
"src/vendors/libraryDetector/**",
"src/**/*.test.ts",
"src/**/*.test.js",
"node_modules",
".idea",
"browsers/dist",
"artifacts",
"scripts/bin",
"src/vendors",
"src/support.js"
],
"overrides": [
{
"files": [
"webpack.*.js",
"*.config.js",
"test-env.js",
"**/__mocks__/**",
"*.test.js"
],
"env": {
"node": true,
"jest": true
},
"rules": {
"@typescript-eslint/no-var-requires": "off"
}
}
twschiller marked this conversation as resolved.
Show resolved Hide resolved
]
}
17 changes: 17 additions & 0 deletions .github/workflows/integration.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,3 +63,20 @@ jobs:
name: brick-headers
path: headers.json
retention-days: 5

lint:
runs-on: ubuntu-latest

steps:
- uses: actions/checkout@v2
- uses: actions/setup-node@v1
with:
node-version: "14.x"
- uses: bahmutov/npm-install@v1
# Run eslint without GitHub Actions annotations
# https://stackoverflow.com/a/65964721/288906
- name: npm run lint
run: |
echo "::remove-matcher owner=eslint-compact::"
echo "::remove-matcher owner=eslint-stylish::"
npm run lint
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Codacy only tracks changes, but not existing files. I added this CI job to ensure that all files are affected by new rules.

Possible improvements:

  • Run this job only if the lockfile or eslint config has changed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The workflow would be this, if interested, but I think it's best to just leave it running at all times. It costs nothing and it already passes thanks to this PR 🎉

lint.yml 
name: Lint

on:
  push:
    branches:
      - "*"
    path:
      - .eslintrc
      - package-lock.json

jobs:
  lint:
    runs-on: ubuntu-latest

    steps:
      - uses: actions/checkout@v2
      - uses: actions/setup-node@v1
        with:
          node-version: "14.x"
      - uses: bahmutov/npm-install@v1

      # Run eslint without GitHub Actions annotations
      # https://stackoverflow.com/a/65964721/288906
      - name: npm run lint
        run: |
          echo "::remove-matcher owner=eslint-compact::"
          echo "::remove-matcher owner=eslint-stylish::"
          npm run lint

1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ db.sqlite3
.env
.env.*
staticfiles
.vscode

# User-specific stuff:
.idea/workspace.xml
Expand Down
4 changes: 4 additions & 0 deletions browsers/webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,10 @@ module.exports = (env, options) => ({
type: "json", // Required by Webpack v4
use: "yaml-loader",
},
{
test: /\.txt/,
type: "asset/source",
},
],
},
});
88 changes: 12 additions & 76 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
"scripts": {
"test": "jest",
"test:watch": "jest --watchAll",
"lint": "eslint src/**",
"lint": "eslint src",
"watch": "ENV_FILE='.env.development' webpack --mode development --config browsers/webpack.config.js --watch",
"build": "NODE_OPTIONS=--max_old_space_size=8192 webpack --mode production --config browsers/webpack.config.js",
"build:scripts": "webpack --mode production --config scripts/webpack.scripts.js",
Expand Down Expand Up @@ -156,7 +156,7 @@
"@types/webpack-env": "^1.16.0",
"@types/yup": "^0.29.11",
"@typescript-eslint/eslint-plugin": "^4.27.0",
"@typescript-eslint/parser": "^4.20.0",
"@typescript-eslint/parser": "^4.27.0",
"axios-mock-adapter": "^1.19.0",
"babel-jest": "^27.0.2",
"compass-mixins": "^0.12.10",
Expand Down
1 change: 0 additions & 1 deletion src/devTools/editor/SelectorSelectorField.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ import { Framework } from "@/messaging/constants";
import { reportError } from "@/telemetry/logging";

// eslint is complaining that it can't parse the Option file
// eslint-disable-next-line import/namespace
import { OptionProps } from "react-select/src/components/Option";
import { useToasts } from "react-toast-notifications";

Expand Down
2 changes: 1 addition & 1 deletion src/helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ export function engineRenderer(
}

// first part of the path can be global context with a @
const pathRegex = /^(@?[a-zA-Z0-9_\-]+\??)(\.[a-zA-Z0-9_\-]+\??)*$/;
const pathRegex = /^(@?[a-zA-Z0-9_-]+\??)(\.[a-zA-Z0-9_-]+\??)*$/;

/**
* Return true if maybePath refers to a property in ctxt.
Expand Down
5 changes: 5 additions & 0 deletions src/import-svg.d.ts → src/import-assets.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,8 @@ declare module "*.svg" {
const value: any;
export default value;
}

declare module "*.txt" {
const value: string;
export default value;
}
22 changes: 7 additions & 15 deletions src/options/pages/brickEditor/CodeEditor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,21 +21,13 @@ import Select from "react-select";
import React, { useState, useRef, Suspense, useEffect } from "react";
import { useField, useFormikContext } from "formik";

// https://webpack.js.org/loaders/raw-loader/#examples
const serviceTemplate = require("raw-loader!@contrib/templates/service.txt?esModule=false")
.default;
const emberjsTemplate = require("raw-loader!@contrib/templates/reader-emberjs.txt?esModule=false")
.default;
const jqueryTemplate = require("raw-loader!@contrib/templates/reader-jquery.txt?esModule=false")
.default;
const reactTemplate = require("raw-loader!@contrib/templates/reader-react.txt?esModule=false")
.default;
const menuTemplate = require("raw-loader!@contrib/templates/foundation-menu-item.txt?esModule=false")
.default;
const panelTemplate = require("raw-loader!@contrib/templates/foundation-panel.txt?esModule=false")
.default;
const blueprintTemplate = require("raw-loader!@contrib/templates/blueprint-menu.txt?esModule=false")
.default;
import serviceTemplate from "@contrib/templates/service.txt";
import emberjsTemplate from "@contrib/templates/reader-emberjs.txt";
import jqueryTemplate from "@contrib/templates/reader-jquery.txt";
import reactTemplate from "@contrib/templates/reader-react.txt";
import menuTemplate from "@contrib/templates/foundation-menu-item.txt";
import panelTemplate from "@contrib/templates/foundation-panel.txt";
import blueprintTemplate from "@contrib/templates/blueprint-menu.txt";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the docs:

asset/source exports the source code of the asset. Previously achievable by using raw-loader.

From a quick check of the dist/options.js, it seems to work, but a raw string is exported instead of a function. Can you run this code to ensure it still works?

Before

Screen Shot 21

After

Screen Shot 22

Copy link
Contributor

Choose a reason for hiding this comment

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

@fregante You can test this by 1) going to the workshop, 2) clicking create new brick, and 3) selecting a template from the dropdown

See documentation for more information on accessing: https://docs.pixiebrix.com/developer-guide/workshop-basics#202afa4a0c0b4c449031d30540ba9a5d

Copy link
Contributor Author

@fregante fregante Jun 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks! With your steps I'll find my way around the code base over time. As you said going from the UI to the code is as easy as searching a string, but code -> UI takes a bit more time.

Anyway, tested and it works 👍

Screen Shot


const AceEditor = React.lazy(
() =>
Expand Down