docs(eslint-plugin):improve docs [consistent-type-exports] by kmin-jeong · Pull Request #4792 · typescript-eslint/typescript-eslint

kmin-jeong · 2022-04-07T07:13:04Z

PR Checklist

[v] Addresses an existing issue: fixes [consistent-type-exports] Improve docs #4769
[v] That issue was marked as accepting prs
[v] Steps in CONTRIBUTING.md were taken

Overview

I remove duplicate exports in the "correct" example for fixMixedExportsWithInlineTypeSpecifier. and I add about 'isolatedModules' to "When Not To Use It".
I wish reviewers check to see if it has been modified to fit the context.

nx-cloud · 2022-04-07T07:13:06Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 2f2bb8d. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 43 targets

Sent with 💌 from NxCloud.

typescript-eslint · 2022-04-07T07:13:08Z

Thanks for the PR, @kmin-jeong!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

netlify · 2022-04-07T07:13:11Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`2f2bb8d`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/625372e39c1e2a0009b02954
😎 Deploy Preview	https://deploy-preview-4792--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Josh-Cena

Missing the example in the "rule details" section

Josh-Cena · 2022-04-08T02:28:00Z


 - If you are using a TypeScript version less than 3.8, then you will not be able to use this rule as type exports are not supported.
 - If you specifically want to use both export kinds for stylistic reasons, you can disable this rule.
+- Re-exporting a type when the '--isolatedModules' flag is provided requires using 'export type'.


Probably a minor rewording so it goes more along the lines of "If you use --isolatedModules you don't need this rule since TS would enforce type-export specifiers"

Josh-Cena · 2022-04-08T11:15:48Z

 # [5.18.0](https://github.com/typescript-eslint/typescript-eslint/compare/v5.17.0...v5.18.0) (2022-04-04)

-
+ 


Remove this?

Josh-Cena · 2022-04-08T11:16:59Z

+export interface ButtonProps{
+  onClick: () => void;
+}
+
+class Button implements ButtonProps {
+  onClick(){
+    console.log('button!')
+  }
+}


The example should illustrated re-exporting types. For example:

Suggested change

export interface ButtonProps{

onClick: () => void;

}

class Button implements ButtonProps {

onClick(){

console.log('button!')

}

}

interface ButtonProps {

onClick: () => void;

}

class Button implements ButtonProps {

onClick() {

console.log('button!');

}

}

export { Button };

export type { ButtonProps };

Probably we should also give an invalid example, like export { Button, ButtonProps };

Also, I believe code style can be fixed with Prettier. Please remember to format files before committing.

Josh-Cena · 2022-04-08T11:18:57Z


 - If you are using a TypeScript version less than 3.8, then you will not be able to use this rule as type exports are not supported.
 - If you specifically want to use both export kinds for stylistic reasons, you can disable this rule.
+- If you use --isolatedModules, you don't need this rule since TS would enforce type-export specifiers.


Not critical, but I think it's clearer this way:

Suggested change

- If you use --isolatedModules, you don't need this rule since TS would enforce type-export specifiers.

- If you use `--isolatedModules`, you don't need this rule since the compiler would error if a type is not re-exported using `export type`.

fix examples and --Isolatedmodules explain

Josh-Cena · 2022-04-08T11:53:24Z

+//invalid example
+export { Button, ButtonProps };


Please refer to how other rule docs are written. You should use two tabs, one incorrect and one correct.

@Josh-Cena Should I write more explanations about the wrong example?

one is correct, another is incorrect

JoshuaKGoldberg

Thanks for sending! +1 to what Josh-Cena said, and one little change from me please. 🙂

kmin-jeong · 2022-04-09T05:46:29Z

@JoshuaKGoldberg @Josh-Cena @bradzacher
Is there anything else I need to modify?

Josh-Cena · 2022-04-09T06:00:19Z

 # [5.18.0](https://github.com/typescript-eslint/typescript-eslint/compare/v5.17.0...v5.18.0) (2022-04-04)

-
+ 


Again, I think this needs to be reverted

@Josh-Cena ok! I revert the change log and modify the When Not To Use It part splitting two items now!

Josh-Cena · 2022-04-09T06:00:45Z


 - If you are using a TypeScript version less than 3.8, then you will not be able to use this rule as type exports are not supported.
 - If you specifically want to use both export kinds for stylistic reasons, you can disable this rule.
+- If you use `--isolatedModules` the compiler would error if a type is not re-exported using `export type`. If you also don't wish to enforce one style over the other, you can disable this rule.


Split this into two items

kmin-jeong · 2022-04-10T11:54:44Z

@Josh-Cena Does it still look reverted in changelog.md?

Josh-Cena · 2022-04-10T12:07:10Z

You can check yourself in https://github.com/typescript-eslint/typescript-eslint/pull/4792/files :)

kmin-jeong · 2022-04-10T12:21:49Z

@Josh-Cena I commit the code with the message "fix: revert change log" yesterday after reverting changelog.md.
I compared and fixed the changelog.md in the typescript-eslint branch and the changelog in the kminjeong branch
Is there any sentence that needs to be removed or reverted from the changelog?

Josh-Cena · 2022-04-10T12:23:10Z

You shouldn't change changelog.md at all.

kmin-jeong · 2022-04-10T12:30:44Z

@Josh-Cena
Then, have the required modifications been satisfied?

Josh-Cena · 2022-04-10T12:31:58Z

Yes, this looks good to me. Just waiting for one of the maintainers to review it :)

JoshuaKGoldberg

Thanks @kmin-jeong and @Josh-Cena! This looks great! 🙌

kmin-jeong added 3 commits April 7, 2022 14:42

feat: update repository

5b79028

remove duplicate examples

5c3af4d

feat:add about re-exporting --isolatedModules in When Not To Use It

f5674a8

Josh-Cena requested changes Apr 8, 2022

View reviewed changes

kmin-jeong added 8 commits April 8, 2022 13:57

feat: add exmaples

d47eeb4

fix: remove duplicate examples in correct

6f74fea

fix:correct words

7fb6dc7

fix: fix space-between

e77ae67

fix: fix code

2572fd6

fix: fix code space

40f496f

fix: check error

b508138

fix:missed examples

70dc242

Josh-Cena reviewed Apr 8, 2022

View reviewed changes

feat: read review and fix them

c895ad8

fix examples and --Isolatedmodules explain

Josh-Cena reviewed Apr 8, 2022

View reviewed changes

feat: add two taps

6158e57

one is correct, another is incorrect

Josh-Cena reviewed Apr 8, 2022

View reviewed changes

Comment thread packages/eslint-plugin/docs/rules/consistent-type-exports.md

fix: add 

c4a25fd

JoshuaKGoldberg requested changes Apr 8, 2022

View reviewed changes

Comment thread packages/eslint-plugin/docs/rules/consistent-type-exports.md Outdated

JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Apr 8, 2022

kmin-jeong added 2 commits April 9, 2022 01:40

feat: fix explaination about isolatedmodules

cc71bea

fix: fix explain about isolatedModules

251f49e

kmin-jeong requested a review from JoshuaKGoldberg April 8, 2022 16:50

bradzacher added documentation Documentation ("docs") that needs adding/updating awaiting response Issues waiting for a reply from the OP or another party and removed awaiting response Issues waiting for a reply from the OP or another party labels Apr 9, 2022

bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Apr 9, 2022

Josh-Cena requested changes Apr 9, 2022

View reviewed changes

kmin-jeong added 7 commits April 9, 2022 15:07

fis: modify When no to use it

d42a546

fix: revert change log

f1c3ce4

fix: add lint

4246b6b

fix: add lint

c1af45c

add: fix code

42d8958

fix:fix docs splits

efd3668

feat: add lint

8b4f9de

JoshuaKGoldberg reviewed Apr 10, 2022

View reviewed changes

Comment thread packages/eslint-plugin/docs/rules/consistent-type-exports.md Outdated

Update packages/eslint-plugin/docs/rules/consistent-type-exports.md

24c0f2e

JoshuaKGoldberg approved these changes Apr 10, 2022

View reviewed changes

Merge branch 'main' into master

2f2bb8d

JoshuaKGoldberg merged commit 255eea8 into typescript-eslint:main Apr 11, 2022

github-actions Bot locked as resolved and limited conversation to collaborators May 23, 2022

		# [5.18.0](https://github.com/typescript-eslint/typescript-eslint/compare/v5.17.0...v5.18.0) (2022-04-04)

	- If you use --isolatedModules, you don't need this rule since TS would enforce type-export specifiers.
	- If you use `--isolatedModules`, you don't need this rule since the compiler would error if a type is not re-exported using `export type`.

Uh oh!

Conversation

kmin-jeong commented Apr 7, 2022

PR Checklist

Overview

Uh oh!

nx-cloud Bot commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

☁️ Nx Cloud Report

Uh oh!

typescript-eslint Bot commented Apr 7, 2022

Uh oh!

netlify Bot commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for typescript-eslint ready!

Uh oh!

Josh-Cena left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kmin-jeong commented Apr 9, 2022

Uh oh!

Josh-Cena Apr 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmin-jeong commented Apr 10, 2022

Uh oh!

Josh-Cena commented Apr 10, 2022

Uh oh!

kmin-jeong commented Apr 10, 2022

Uh oh!

Josh-Cena commented Apr 10, 2022

Uh oh!

kmin-jeong commented Apr 10, 2022

Uh oh!

Josh-Cena commented Apr 10, 2022

Uh oh!

Uh oh!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nx-cloud Bot commented Apr 7, 2022 •

edited

Loading

netlify Bot commented Apr 7, 2022 •

edited

Loading

Josh-Cena Apr 9, 2022 •

edited

Loading