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.

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

Updated to minimal standard eslint config #2814

Merged
merged 5 commits into from
Oct 1, 2020
Merged

Updated to minimal standard eslint config #2814

merged 5 commits into from
Oct 1, 2020

Conversation

GeoffCoxMSFT
Copy link
Member

Description

First step to standardizing the minimal lint rules for the TS/JS SDK.

Specific Changes

  • added eslint packages to root for typescript, security, and recommended lint rules
  • add packages and configuration to property chain prettier and eslint
  • updated editorconfig to add standard config
  • removed eslint configuration from individual packages (was causing interference)
  • removed eslint-jsdoc (temporarily) until code lint issues can be fixed (15K of them)
  • kept all linting errors as warnings
  • moved lint related packages to devDependencies

Testing

N/A

@coveralls
Copy link

coveralls commented Sep 30, 2020

Pull Request Test Coverage Report for Build 171019

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 83.62%

Totals Coverage Status
Change from base Build 170823: 0.0%
Covered Lines: 15626
Relevant Lines: 17784

💛 - Coveralls

],
"overrides": [
{
"files": ["libraries/adaptive-expressions/src/*.ts", "libraries/adaptive-expressions/src/**/*.ts"],
Copy link
Contributor

Choose a reason for hiding this comment

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

@stevengum where did we land on dropping jsdoc linting? I thought it might still be useful to include as a warning but I could be mistaken there.

Copy link
Member

Choose a reason for hiding this comment

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

@JonathanFingold can you indicate what the correct practice is here? More specifically which tools and configurations (if any) should we leave in the JS repo for generating the docs?

Copy link
Contributor

Choose a reason for hiding this comment

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

Researching. I'll post here when I know more.

Copy link
Member Author

Choose a reason for hiding this comment

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

My plan is to restore the js-doc - maybe with fewer rule exceptions - once we have the code issues fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

TypeDoc

We use TypeDoc behind the scenes for TypeScript documentation (recommended reading: platform breakdown). To make sure that the content renders properly, please ensure that the TypeScript APIs are using standard TypeDoc conventions, available here.

Linting & structure enforcement

You can use ESLint, in combination with valid-jsdoc to make sure that your JSDoc comments are in good shape.

For TypeScript, you can use tslint, with the completed-docs rule.


Use [link-text](xref:uid) style links to link to other Bot Framework and Microsoft TypeScript ref docs.

To find the xref for JavaScript, go to https://docs.microsoft.com/en-us/javascript/api/ and search for the member you want to link to. The xref will be the link text, as opposed to the link target. For example, the xref for the BotAdapter.use method is botbuilder-core.BotAdapter.use.

For links into our docs, link "directly" to the article, such as https://docs.microsoft.com/azure/bot-service/bot-builder-howto-send-messages for the Send and receive text messages how to.

package.json Outdated
"eslint-plugin-prettier": "^3.1.4",
"eslint-plugin-security": "^1.4.0",
"prettier": "^2.1.2",
"typescript": "^3.9.7"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we're using this version of Typescript yet (unfortunately 😢), I suspect this is the version that Prettier is using for parsing? It would probably be best to keep this consistent with whatever is most widely used across all the libraries.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can remove it for now. Maybe it is better defined per library.

Copy link
Member

Choose a reason for hiding this comment

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

I like the idea of a peerDependency in each package; with the root package.json having a it as a devDependency. Right now we're pinned to 3.5.3 because 3.6 introduced some breaking changes. @joshgummersall is investigating our migration story for TypeScript.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think peerDependency is used to express "runtime dependencies" rather than build time dependencies. We don't want to express typescript a runtime dependency because the compiled JS we're publishing has no dependency on typescript. IIRC, lerna supports hoisted dependencies such that we could list [email protected] in the root package.json and a different one in packages that differ. That's how yarn workspaces functionality works.

Copy link
Member Author

Choose a reason for hiding this comment

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

npm behavior changed to not restore peerDependencies (quite awhile ago I think). Composer is using peer dependencies to avoid including multiple different versions of a package when one has ^1.0.1 and another has ^1.0.2. We don't need to use peerDependencies for devDependencies though as peerDependenicies are only resolved through the top level dependencies list.

Copy link
Member

Choose a reason for hiding this comment

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

We use this tool in azure-sdk-for-js to support below TS 3.6 in our public typings: https://github.com/sandersn/downlevel-dts

Copy link
Contributor

@joshgummersall joshgummersall Sep 30, 2020

Choose a reason for hiding this comment

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

@xirzec that's great news! That's exactly the tool I was looking at.

Copy link
Member

@stevengum stevengum left a comment

Choose a reason for hiding this comment

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

:shipit: :shipit: :shipit:

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.

6 participants