Skip to content

fix(eslint-plugin): remove imports from typescript-estree #706

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
Merged
merged 1 commit into from
Jul 16, 2019
Merged

fix(eslint-plugin): remove imports from typescript-estree #706

merged 1 commit into from
Jul 16, 2019

Conversation

mikeharder
Copy link
Contributor

bradzacher
bradzacher requested changes Jul 15, 2019
View reviewed changes
Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

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

This is the wrong way to do it.

There is no reason to have a dependency on typescript-estree, as everything that should be required is exposed via experimental-utils.

The better place to fix this is the one place it's erroring: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/src/rules/prefer-readonly.ts#L5

@mikeharder
Copy link
Contributor Author

@bradzacher: Are you suggesting the following change at this location?

typescript-eslint/packages/eslint-plugin/src/rules/prefer-readonly.ts

Line 5 in 76b89a5

import { TSESTree, AST_NODE_TYPES } from '@typescript-eslint/typescript-estree'; 

- import { TSESTree, AST_NODE_TYPES } from '@typescript-eslint/typescript-estree';
+ import { TSESTree, AST_NODE_TYPES } from '@typescript-eslint/experimental-utils';

If so, should this rule be changed in the same way?

typescript-eslint/packages/eslint-plugin/src/rules/prefer-regexp-exec.ts

Line 1 in 4edf0d7

import { TSESTree } from '@typescript-eslint/typescript-estree';

Finally, what about this rule?

typescript-eslint/packages/eslint-plugin/src/rules/triple-slash-reference.ts

Lines 2 to 7 in af70a59

import {
Literal,
Node,
TSExternalModuleReference,
} from '@typescript-eslint/typescript-estree/dist/ts-estree/ts-estree';
import { TSESTree } from '@typescript-eslint/typescript-estree';

@bradzacher
Copy link
Member

yes, that's what I mean!

experimental-utils was setup to be the one-stop-shop for plugins written in typescript - it depends on typescript-estree and re-exports the relevant pieces, so that should be the only one ever used by the plugin.

Unfortunately VSCode's auto importer likes to suggest packages that aren't direct dependencies, and it's hard to catch that error in reviews. There are some lint fixes in the pipeline to catch this in future.

It looks like you already have done it, but could you please make sure that there are no results for this grep from '@typescript-eslint/typescript-estree in the eslint-plugin?

@mikeharder
Copy link
Contributor Author

@bradzacher: The 3 rules I linked above are the only I found with from '@typescript-eslint/typescript-estree. The first two should be easy to fix by replacing typescript-estree with experimental-utils, but triple-slash-reference.ts looks more difficult since it's importing types which don't appear to be exported from experimental-utils.

@bradzacher
Copy link
Member

Having a look, all 3 should be from the TSESTree export in experimental-utils

 import { 
   Literal,                    // TSEStree.Literal
   Node,                       // TSESTree.Node
   TSExternalModuleReference,  // TSEStree.TSExternalModuleReference
 } from '@typescript-eslint/typescript-estree/dist/ts-estree/ts-estree';

@mikeharder
Copy link
Contributor Author

I think I know the changes required then, will update this PR.

@mikeharder mikeharder force-pushed the eslint-plugin-depends-typescript-estree branch from 78b9d7f to 3dee0d2 Compare July 15, 2019 22:57
@mikeharder mikeharder force-pushed the eslint-plugin-depends-typescript-estree branch from 3dee0d2 to cad6baa Compare July 15, 2019 22:59
@codecov
Copy link

codecov bot commented Jul 15, 2019 •
edited
Loading

Codecov Report

Merging #706 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master     #706   +/-   ##
=======================================
  Coverage   94.33%   94.33%           
=======================================
  Files         111      111           
  Lines        4593     4593           
  Branches     1268     1268           
=======================================
  Hits         4333     4333           
  Misses        149      149           
  Partials      111      111
Impacted Files Coverage Δ
...ages/eslint-plugin/src/rules/prefer-regexp-exec.ts 100% <ø> (ø) ⬆️
.../eslint-plugin/src/rules/triple-slash-reference.ts 90% <100%> (ø) ⬆️
...ackages/eslint-plugin/src/rules/prefer-readonly.ts 99% <100%> (ø) ⬆️

@mikeharder mikeharder changed the title fix(eslint-plugin): add dependency typescript-estree fix(eslint-plugin): remove imports from typescript-estree Jul 16, 2019
@mikeharder
Copy link
Contributor Author

@bradzacher: I believe I have updated this PR with the changes you requested.

bradzacher
bradzacher approved these changes Jul 16, 2019
View reviewed changes
Copy link
Member

@bradzacher bradzacher left a comment •
edited
Loading

Choose a reason for hiding this comment

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

perfect! thanks for doing this!

@bradzacher bradzacher merged commit ceb2d32 into typescript-eslint:master Jul 16, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bradzacher bradzacher bradzacher approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

eslint-plugin should not import from typescript-estree
2 participants
@mikeharder @bradzacher