Replace TSLint with ESLint #1325

filmaj · 2021-09-01T18:53:05Z

Work In Progress!

Summary

This is a first go at introducing the ESLint configs we converged on in the bolt-js project to this project. Did you all think you would be free of thousands of lines of changes moving commas around and introducing newline characters? Well think again! 😈

The initial pull request includes changes that will be core and applicable to all published packages making up this repository as well as changes to packages/web-api/src code to have the web-api package adhere to the new lint configuration. As a result, the most important changes to review include:

scripts/code_generator.rb: changes to the type generator script (replacing tslint:disable with eslint-disable)
Copies ESLint config files from bolt-js into a new lint-configs/ directory
What do people think of the approach of symlinking the eslint config files into each published package's directory? Any alternatives or better approaches that we should consider?
- Related: because the eslint config files defining the rules pull in certain dependencies, this requires putting some of the eslint dependencies (like eslint itself and the jsdoc rules) into the top-level package.

Summary of changes so far:

Updated type/code generator (scripts/code_generator.rb) to drop in an eslint-disable directive into generated code instead of a tslint:disable one.
Factored out eslint-related configs into a top-level lint-configs directory.
Set up symlinks to eslint configuration files between packages/web-api and lint-configs directories.

Questions

Symlink approach good enough?
Should the top-level package.json differentiate between linting vs. testing each sub-project? i.e. lerna run lint and lerna run test being separate? Alternatively, can include linting in each sub-project's test npm run script, and drop the execution of lerna run lint. IMO the latter makes sense as passing the linter should be considered necessary - and thus linting is implicitly part of the test suite.

TODO

Do an audit / search for tslint:disable directives
Review .npmignore files to make sure published packages don't include any of the new eslint config files
Instructions need to include running npm i from the root directory to ensure you have eslint-plugin-jsdoc installed

Requirements (place an `x` in each `[ ]`)

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.

seratch · 2021-09-03T00:06:48Z

Thanks for working on this! Looks good so far!

…olt-js to node-slack-sdk. Updated type/code generator to drop in an eslint-disable directive into generated code instead of a tslint:disable one. Factored out eslint-related configs into a top-level lint-configs directory.

…from logger package.

…ctives.

…uns as part of testing. Should save some CI time.

srajiang

Looking good! Left a couple of questions/ comments! Also, are you planning on committing up a package-lock.json with these changes?

srajiang · 2021-09-09T16:39:09Z

packages/oauth/src/index.ts

 import { Logger, LogLevel, getLogger } from './logger';
 import { MemoryInstallationStore } from './stores';

+// default implementation of StateStore


Out of curiosity, what's the reasoning behind moving this up? Was this related to one of the eslint rules?

Yes, the 'no use before defined' rule. While functions are hoisted to the top by the runtime, classes, and I guess types as well, are not.

srajiang · 2021-09-09T16:42:05Z

packages/oauth/src/index.ts

-        parsedUrl = parseUrl(req.url, true);
-        code = parsedUrl.query.code as string;
-        state = parsedUrl.query.state as string;
-        if (state === undefined || state === '' || code === undefined) {


Wondering whether this was written this way to be a bit more explicit for people unfamiliar with the JS type coercion.

I rewrote this part as use of parseUrl is deprecated and that triggered an eslint rule! So I did it the 'approved' newer-node-version way of creating a new URL.

srajiang · 2021-09-09T16:42:37Z

packages/oauth/src/index.ts

@@ -817,7 +823,7 @@ export interface OAuthV2TokenRefreshResponse extends WebAPICallResult {
  team: { id: string, name: string };
  enterprise: { name: string, id: string } | null;
  is_enterprise_install: boolean;
-  response_metadata: {}; // TODO
+  response_metadata: Record<string, never>; // TODO


Is there a reason to keep this comment in this line now?

Good question! I think so but am not sure - Record seems like a barely-more-acceptable type than {}?

Suggested change

response_metadata: Record<string, never>; // TODO

response_metadata: Record<string, never>; // TODO revisit this type to see whether there are better alternatives

The property can exist but actually the value is not used. The internal type def is an alias of https://github.com/slackapi/node-slack-sdk/blob/main/packages/web-api/src/response/OauthV2AccessResponse.ts See also: https://github.com/slackapi/node-slack-sdk/blob/%40slack/oauth%402.2.0/packages/oauth/src/index.ts#L209-L214

You can safely remove the property. Let's remove it! If you hesitate to have potentially breaking changes in this PR, update the TODO comment to inclue my comment here.

Sounds good, I have removed it in the most recent commit!

seratch

No other comments! Thanks for the massive efforts here 👍

seratch · 2021-09-10T01:46:40Z

packages/oauth/src/index.spec.js

@@ -398,7 +398,7 @@ describe('OAuth', async () => {
    });

    it('should call the failure callback due to missing state query parameter on the URL', async () => {
-      const req = { url: 'someUrl' };
+      const req = { url: 'http://someUrl.com' };


It seems that this domain is used. Just in case, how about using example.com?

Yes, good idea. In fact, this change is related to another change (see my related review comment here). Because of the removal of the use of parseUrl (which is deprecated) and replacing it with new URL(), if the protocol and/or TLD is not provided in the URL string, it will cause an error. I wonder if this has farther-reaching consequences and as a result need to add more tests for this? For example, what if someone has custom domain mappings in their e.g. hosts file, to map e.g. someUrl to 127.0.0.1. In the current implementation, perhaps it is perfectly OK to use a technically malformed URL (no protocol and/or TLD), whereas due to this change, that would not be acceptable.

What do you think?

Thanks for sharing the context. My comment was not clear enough but I wanted to suggest http://example.com or https://example.com instead. If you click the URL in your editor, you will be navigated to the URL. Some people may unintentionally access the URLs.

Although I don't have any thoughts on the domain itself, I think that we can always consider using the safest domains / URLs in test data. I'm not stick with example.com at all but it's commonly used. For this reason, it should be good enough.

seratch · 2021-09-10T01:59:37Z

packages/oauth/src/index.ts

@@ -817,7 +823,7 @@ export interface OAuthV2TokenRefreshResponse extends WebAPICallResult {
  team: { id: string, name: string };
  enterprise: { name: string, id: string } | null;
  is_enterprise_install: boolean;
-  response_metadata: {}; // TODO
+  response_metadata: Record<string, never>; // TODO


The property can exist but actually the value is not used. The internal type def is an alias of https://github.com/slackapi/node-slack-sdk/blob/main/packages/web-api/src/response/OauthV2AccessResponse.ts See also: https://github.com/slackapi/node-slack-sdk/blob/%40slack/oauth%402.2.0/packages/oauth/src/index.ts#L209-L214

You can safely remove the property. Let's remove it! If you hesitate to have potentially breaking changes in this PR, update the TODO comment to inclue my comment here.

filmaj added the tests M-T: Testing work only label Sep 1, 2021

filmaj requested review from seratch, stevengill, misscoded and srajiang September 1, 2021 18:53

filmaj self-assigned this Sep 1, 2021

filmaj requested a review from mwbrooks September 1, 2021 18:53

This was referenced Sep 1, 2021

introduce ESLint at top-level and unify tsconfigs #808

Closed

Tslint to eslint #1074

Closed

filmaj marked this pull request as ready for review September 2, 2021 12:34

filmaj added 13 commits September 8, 2021 14:12

ESLint applied to webhook package.

f90e4d2

ESLint applied to types package.

4264b74

ESLint applied to socket-mode package.

ba41896

ESLint applied to rtm-api package.

7ec10f5

ESLint applied to oauth package.

d397aa9

Fixing object type expectation on parameter.

cd272aa

ESLint applied to logger package.

701f24b

ESLint applied to interactive-messages package, removing unused deps …

93ccc58

…from logger package.

Bizarre linting error i cant repro?

197ee2c

ESLint applied to events-api package

37428af

ESLint applied to client package

7eadc80

Updating nock and mocha dev dependencies to recent versions

84f0731

filmaj force-pushed the tslint-to-eslint branch from 4b71893 to 84f0731 Compare September 8, 2021 18:12

filmaj added 4 commits September 8, 2021 14:53

Changes required due to major version upgrade of nock.

89b0e92

Remove no longer needed tslint.json files, remove tslint comment dire…

f7a8f5e

…ctives.

Tweaking maintainer guide.

bd2ce11

Dont run linting separate from testing via lerna, since linting now r…

eddfc7d

…uns as part of testing. Should save some CI time.

srajiang approved these changes Sep 9, 2021

View reviewed changes

srajiang mentioned this pull request Sep 10, 2021

Add optional InstallProvider#stateVerification flag #1329

Merged

6 tasks

seratch approved these changes Sep 10, 2021

View reviewed changes

filmaj added 2 commits September 13, 2021 11:12

Removing unused response_metadata property

d3aec0a

use example.com for example URLs.

44010b6

filmaj merged commit 542e5f9 into main Sep 13, 2021

filmaj deleted the tslint-to-eslint branch September 13, 2021 19:16

hello-ashleyintech added this to the rtm-api@6.1.0 milestone Feb 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace TSLint with ESLint #1325

Replace TSLint with ESLint #1325

filmaj commented Sep 1, 2021 •

edited

seratch commented Sep 3, 2021

srajiang left a comment

srajiang Sep 9, 2021

filmaj Sep 9, 2021

srajiang Sep 9, 2021

filmaj Sep 9, 2021

srajiang Sep 9, 2021

filmaj Sep 9, 2021

srajiang Sep 9, 2021

seratch Sep 10, 2021

filmaj Sep 13, 2021

seratch left a comment

seratch Sep 10, 2021

filmaj Sep 13, 2021

seratch Sep 13, 2021

seratch Sep 10, 2021

	response_metadata: Record<string, never>; // TODO
	response_metadata: Record<string, never>; // TODO revisit this type to see whether there are better alternatives

Replace TSLint with ESLint #1325

Replace TSLint with ESLint #1325

Conversation

filmaj commented Sep 1, 2021 • edited

Summary

Questions

TODO

Requirements (place an x in each [ ])

seratch commented Sep 3, 2021

srajiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seratch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filmaj commented Sep 1, 2021 •

edited

Requirements (place an `x` in each `[ ]`)