Skip to content

Add eslint-config-azuretools package#972

Merged
wwlorey merged 7 commits intomainfrom
wwl/eslint-package
Aug 10, 2021
Merged

Add eslint-config-azuretools package#972
wwlorey merged 7 commits intomainfrom
wwl/eslint-package

Conversation

@wwlorey
Copy link
Contributor

@wwlorey wwlorey commented Jul 29, 2021

I tried exporting an ESLint config file from the dev package but ESLint doesn’t currently support loading a module as a config file. There's an open issue to support that but it doesn't seem like it's getting traction: eslint/eslint#14170

So I'm proposing we make a little npm package that's just our ESLint configuration. This is the recommended way to share ESLint configs (source).

To use this in each extension, we would add "eslint-config-azuretools" as a dependency and ".eslintrc.js" would be simplified to:

module.exports = {
    extends: "azuretools"
};

(ESLint assumes the "eslint-config-" part)

If this is the direction we want to go, I'll add the rest of the files needed to make this a for-real package (license, readme, etc.)

@wwlorey wwlorey requested a review from a team as a code owner July 29, 2021 18:19
"@typescript-eslint/ban-types": "off",
"@typescript-eslint/require-await": "off",
"no-constant-condition": ["error", { "checkLoops": false }],
"eqeqeq": ["error", "always"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used functions' ESLint config as the base for this. The only change is this line which was requested here

Copy link
Member

Choose a reason for hiding this comment

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

Two things:

  1. We have test specific ESLint configs for some (I know at least functions and SWA) extensions. Could we create a shared config for these as well?
  2. I recently added two rules to the SWA extension in this PR: Add no-restricted-imports rule to help prevent wrong ext imports vscode-azurestaticwebapps#456. As Eric mentioned on that PR, It would be great to include these rules in the shared config. Do you want to include these rules in the shared config now, or later once the config is implemented and used by the extensions?

Note: adding this rule requires tweaking (see here for what I added for SWA) to the test ESLint config to prevent errors in tests. I quickly tested out adding these rules to functions and the required tweak was similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have test specific ESLint configs for some (I know at least functions and SWA) extensions. Could we create a shared config for these as well?

Our ESLint configs for tests are less standardized than those for production code. I'm open to coming up with a single config for these but I think it might be overkill. I'm curious to see what the rest of the team thinks about this though.

For reference, here's some of our test config files to compare:
https://github.com/microsoft/vscode-azureappservice/blob/main/test/.eslintrc.js
https://github.com/microsoft/vscode-azurefunctions/blob/main/test/.eslintrc.js
https://github.com/microsoft/vscode-cosmosdb/blob/main/test/.eslintrc.js
https://github.com/microsoft/vscode-azurestaticwebapps/blob/main/test/.eslintrc.js
https://github.com/microsoft/vscode-azurevirtualmachines/blob/main/test/.eslintrc.js

I recently added two rules to the SWA extension in this PR: Add no-restricted-imports rule to help prevent wrong ext imports vscode-azurestaticwebapps#456. As Eric mentioned on that PR, It would be great to include these rules in the shared config. Do you want to include these rules in the shared config now, or later once the config is implemented and used by the extensions?

Thanks for bringing this up, I'll go ahead and add them now

@nturinski
Copy link
Member

nturinski commented Jul 30, 2021

After doing a little bit of my own local testing, here's what I found.

  1. If I try to export all the properties we need in the index.js of vscode-azuredevextension, then the ESLint ConfigValidator.validateConfigSchema throws an error about having an invalid ESLint configuration.
  2. Based on the documentation, if we want to extend the eslint module via npm, I think that module.exports has to live in the index.js.
  3. I was able to connect it locally with a file I named eslint.js and it worked fine. I think if we wanted it to work across our other extensions, we could refer to the eslint.js file path directly with a relative path that would look like "./node_modules/vscode-azureextensiondev/src/eslint/eslint.js". I imagine if we published a new dev package, we could refer it to the same way, but it's definitely less clean than just hosting a npm package dedicated to the eslint rules.

@wwlorey
Copy link
Contributor Author

wwlorey commented Aug 9, 2021

As discussed in standup Friday - the plan is to publish our shared config as an npm package for both production & test configs

@wwlorey
Copy link
Contributor Author

wwlorey commented Aug 9, 2021

It turns out you can put multiple configs in one package so I went ahead and added a shared config for tests

@wwlorey wwlorey merged commit 467a9b4 into main Aug 10, 2021
@wwlorey wwlorey deleted the wwl/eslint-package branch August 10, 2021 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants