Feat#Provide a quickfix for non-exported types #37980

Qiyu8 · 2020-04-15T08:57:47Z

if the locals originate from a .d.ts file, do nothing.
No export found in the file:

The declaration contains multiple variables, creating export {...}
The declaration is a single function or variable: add export

more than one export {...} found in the file:

The declaration contains multiple variables or The declaration is a single variable, add the declaration to the last export {...}
The declaration is a single function: add export

tests/cases/fourslash/codeFixImportNonExportedMember.ts

DanielRosenwasser

Very good start! I would also like to see the case where the locals originate from a .d.ts file. We shouldn't provide a quick fix in that case.

src/compiler/diagnosticMessages.json

src/services/codefixes/fixImportNonExportedMember.ts

Qiyu8 · 2020-04-28T08:19:53Z

@DanielRosenwasser Feel free to review if you have time

src/services/codefixes/fixImportNonExportedMember.ts

tests/cases/fourslash/codeFixImportNonExportedMember_all.ts

Co-authored-by: Daniel Rosenwasser <DanielRosenwasser@users.noreply.github.com>

sandersn

I agree that the message needs to change. I think fixing getAllCodeActions is difficult and probably less likely to be used than the individual fix anyway.

tests/cases/fourslash/codeFixImportNonExportedMember_all.ts

src/compiler/diagnosticMessages.json

sandersn · 2020-05-21T17:55:13Z

@Qiyu8 do you want to keep working on this? I think the main thing left to fix is the fix-all message.

Qiyu8 · 2020-05-22T03:41:01Z

@sandersn Please feel free to merge right now ,write a custom getAllCodeActions handler is a little diffcult for me.

Qiyu8 · 2020-05-30T06:26:05Z

@sandersn @DanielRosenwasser Is there anything I can do?

Co-authored-by: Daniel Rosenwasser <DanielRosenwasser@users.noreply.github.com>

Merge branch 'master' of github.com:microsoft/TypeScript into quickfix_non_exported

…x_non_exported

Qiyu8 · 2020-08-04T08:12:18Z

Sorry for the delay, I spent a lot of time in rebase + squash and finally all of the errors and requirements has fulfilled, @elibarzilay @DanielRosenwasser , Please feel free to review.

DanielRosenwasser

This looks like it's getting very close to merging!

src/services/codefixes/fixImportNonExportedMember.ts

DanielRosenwasser · 2020-08-05T06:15:26Z

src/services/codefixes/fixImportNonExportedMember.ts

+        const statements = sourceFile.statements.filter(isExportDeclaration);
+        for (const statement of statements) {
+            if (statement.exportClause && isNamedExports(statement.exportClause)) {
+                namedExport = statement;


Suggested change

namedExport = statement;

return statement;

I want to get the last export { } and add the missing export there, so I can't return immediately.

src/services/codefixes/fixImportNonExportedMember.ts

DanielRosenwasser · 2020-08-05T06:18:50Z

src/services/codefixes/fixImportNonExportedMember.ts

+            return compareIdentifiers(s1.propertyName || s1.name, s2.propertyName || s2.name) ||
+                compareIdentifiers(s1.name, s2.name);


Why do you need to have the fallback here? Why can't this just be the following?

Suggested change

return compareIdentifiers(s1.propertyName || s1.name, s2.propertyName || s2.name) ||

compareIdentifiers(s1.name, s2.name);

return compareIdentifiers(s1.propertyName || s1.name, s2.propertyName || s2.name);

DanielRosenwasser · 2020-08-05T06:21:56Z

src/services/codefixes/fixImportNonExportedMember.ts

+        }
+        if (isFunctionSymbol(localSymbol)) {
+            const start = localSymbol.valueDeclaration.pos;
+            changes.insertExportModifierAt(sourceFile, start ? start + 1 : 0);


What exactly is with the start + 1? I don't understand.

I want to add an export identifier in front of non-exported type, you can see the codeFixImportNonExportedMember1.ts, an export added before function bar(), the start there is the position of line break, so need to +1 for start.

The problem you are running into is more about using the pos (a.k.a. the "full start") instead of the start after the trivia (what you'd get when you call getStart(sourceFile)).

As far as I can tell, a test like

/** * hello */ function bar() { }

will likely end up with the export inside of the comment..

What happens when you just use insertExportModifier instead of insertExportModifierAt?

DanielRosenwasser · 2020-08-05T06:22:12Z

src/services/codefixes/fixImportNonExportedMember.ts

+            return;
+        }
+        if (isFunctionSymbol(localSymbol)) {
+            const start = localSymbol.valueDeclaration.pos;


If you're suing pos, that means you're including leading trivia.

DanielRosenwasser · 2020-08-05T06:25:26Z

src/services/textChanges.ts

+            this.insertExportModifierAt(sourceFile, node.getStart(sourceFile));
+        }
+
+        public insertExportModifierAt(sourceFile: SourceFile, position: number): void {


I just don't quite know if you need this, but I don't understand the start + 1 thing yet.

I want to add an export identifier in front of non-exported type, you can see the codeFixImportNonExportedMember1.ts, an export added before function bar(), the start there is the position of line break, so need to +1 for start.

DanielRosenwasser · 2020-08-15T01:06:47Z

@typescript-bot pack this

typescript-bot · 2020-08-15T01:06:50Z

Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 85d5b4a. You can monitor the build here.

typescript-bot · 2020-08-15T01:23:14Z

Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/82348/artifacts?artifactName=tgz&fileId=C75D4A6B47D650F118247EF885FAA78EC5E08E5280AA0943834AB65938014A6002&fileName=/typescript-4.1.0-insiders.20200815.tgz"
    }
}

and then running npm install.

Qiyu8 · 2020-08-21T06:17:27Z

@DanielRosenwasser I think this PR is ready for merge, What else do I need to do?

DanielRosenwasser · 2020-08-21T06:20:18Z

@typescript-bot pack this

typescript-bot · 2020-08-21T06:20:21Z

Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 85d5b4a. You can monitor the build here.

DanielRosenwasser · 2020-08-21T06:21:44Z

@DanielRosenwasser I think this PR is ready for merge, What else do I need to do?

#37980 (comment) is still something I am wondering about. I'm not seeing why the existing method didn't suffice here.

typescript-bot · 2020-08-21T06:38:44Z

Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/82931/artifacts?artifactName=tgz&fileId=10316357496660C77808C8DB7BEDFBBBA34AE694AE155125DAEC98B186C3F22A02&fileName=/typescript-4.1.0-insiders.20200821.tgz"
    }
}

and then running npm install.

There is also a playground for this build.

DanielRosenwasser

This is the current behavior I see. Please add a relevant test to ensure the export keyword doesn't get placed before the comment. I've already left some feedback on how you can achieve that behavior.

sandersn · 2020-10-28T18:53:39Z

@Qiyu8 do you want to keep working on this?

Qiyu8 · 2020-10-30T03:15:52Z

@sandersn Thanks for your caring, I think the only problem is to ensure the export keyword doesn't get placed before the comment as @DanielRosenwasser pointed out, But I didn't find a solution to do that.

sandersn · 2020-10-30T23:13:45Z

@Qiyu8 I haven't had good experience with comment trivia myself, so maybe you can look at similar codefixes or refactors and then try @DanielRosenwasser 's suggestions?

Qiyu8 · 2020-11-02T03:02:32Z

@DanielRosenwasser How to get the DeclarationStatement of a FunctionSymbol?

if (isFunctionSymbol(localSymbol) && isDeclarationStatement(current.?)) {
            changes.insertExportModifier(sourceFile, current.?);
            return;
        }

I want to add export in front of a function.

sandersn · 2021-02-16T23:51:33Z

@Qiyu8 Sorry for dropping this for so long. I understand if you don't want to work on this anymore.

To answer your latest question: s.valueDeclaration is the value declaration of the symbol, which is the one you want for a function (since a function is a value, not a type.)

sandersn · 2021-03-11T15:05:35Z

This PR hasn't seen any activity for quite a while, so I'm going to close it to keep the number of open PRs manageable. Please let me know if you want to keep working on it and if you need more help to find the declaration of a function.

Qiyu8 added 2 commits April 15, 2020 16:53

Feat#Provide a quickfix for non-exported types

acba122

fix import start pos

3f17337

DanielRosenwasser reviewed Apr 16, 2020

View reviewed changes

tests/cases/fourslash/codeFixImportNonExportedMember.ts Outdated Show resolved Hide resolved

DanielRosenwasser requested changes Apr 16, 2020

View reviewed changes

src/compiler/diagnosticMessages.json Outdated Show resolved Hide resolved

src/services/codefixes/fixImportNonExportedMember.ts Outdated Show resolved Hide resolved

fix according to multiple cases

2b39b01

Qiyu8 requested a review from DanielRosenwasser April 22, 2020 06:20

Qiyu8 marked this pull request as draft April 27, 2020 06:11

Qiyu8 added 2 commits April 28, 2020 15:14

code reconstruct

3a0b235

modify testcase

7f7ab7e

Qiyu8 marked this pull request as ready for review April 28, 2020 07:26

modify testcase message

4a0b533

sandersn added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label May 5, 2020

sandersn assigned DanielRosenwasser May 5, 2020

DanielRosenwasser reviewed May 5, 2020

View reviewed changes

src/services/codefixes/fixImportNonExportedMember.ts Outdated Show resolved Hide resolved

DanielRosenwasser requested changes May 5, 2020

View reviewed changes

tests/cases/fourslash/codeFixImportNonExportedMember_all.ts Show resolved Hide resolved

DanielRosenwasser requested review from elibarzilay and orta May 5, 2020 22:04

Qiyu8 and others added 3 commits May 6, 2020 10:50

Update src/services/codefixes/fixImportNonExportedMember.ts

75cc909

Co-authored-by: Daniel Rosenwasser <DanielRosenwasser@users.noreply.github.com>

add space

83d07eb

Update fixImportNonExportedMember.ts

223d896

sandersn requested changes May 21, 2020

View reviewed changes

tests/cases/fourslash/codeFixImportNonExportedMember_all.ts Show resolved Hide resolved

src/compiler/diagnosticMessages.json Outdated Show resolved Hide resolved

Update fixImportNonExportedMember.ts

e4f120b

Qiyu8 requested a review from sandersn May 25, 2020 01:30

Qiyu8 and others added 6 commits July 1, 2020 11:37

Update src/services/codefixes/fixImportNonExportedMember.ts

3ee7a66

Co-authored-by: Daniel Rosenwasser <DanielRosenwasser@users.noreply.github.com>

Update src/services/codefixes/fixImportNonExportedMember.ts

9f7dcd9

Co-authored-by: Daniel Rosenwasser <DanielRosenwasser@users.noreply.github.com>

update

ed01bbf

Merge branch 'master' of github.com:microsoft/TypeScript into quickfix_non_exported

Merge branch 'master' of github.com:microsoft/TypeScript into quickfi…

09db982

…x_non_exported

add order and fix CI errors

2289ea0

fix lint error

5a68eb2

Qiyu8 requested review from elibarzilay and DanielRosenwasser August 5, 2020 01:50

DanielRosenwasser requested changes Aug 5, 2020

View reviewed changes

review update

85d5b4a

Qiyu8 requested a review from DanielRosenwasser August 6, 2020 12:22

DanielRosenwasser requested changes Aug 21, 2020

View reviewed changes

sandersn closed this Mar 11, 2021

Richard-Lynch mentioned this pull request Jun 16, 2022

Feat#Provide a quick-fix for non-exported imports bloomberg/TypeScript#72

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat#Provide a quickfix for non-exported types #37980

Feat#Provide a quickfix for non-exported types #37980

Qiyu8 commented Apr 15, 2020 •

edited by DanielRosenwasser

Loading

DanielRosenwasser left a comment

Qiyu8 commented Apr 28, 2020

sandersn left a comment

sandersn commented May 21, 2020

Qiyu8 commented May 22, 2020

Qiyu8 commented May 30, 2020

Qiyu8 commented Aug 4, 2020

DanielRosenwasser left a comment

DanielRosenwasser Aug 5, 2020

Qiyu8 Aug 5, 2020

DanielRosenwasser Aug 5, 2020

DanielRosenwasser Aug 5, 2020

Qiyu8 Aug 5, 2020

DanielRosenwasser Aug 15, 2020

DanielRosenwasser Aug 5, 2020

DanielRosenwasser Aug 5, 2020

Qiyu8 Aug 5, 2020

DanielRosenwasser commented Aug 15, 2020

typescript-bot commented Aug 15, 2020 •

edited

Loading

typescript-bot commented Aug 15, 2020

Qiyu8 commented Aug 21, 2020

DanielRosenwasser commented Aug 21, 2020

typescript-bot commented Aug 21, 2020 •

edited

Loading

DanielRosenwasser commented Aug 21, 2020

typescript-bot commented Aug 21, 2020 •

edited

Loading

DanielRosenwasser left a comment •

edited

Loading

sandersn commented Oct 28, 2020

Qiyu8 commented Oct 30, 2020

sandersn commented Oct 30, 2020

Qiyu8 commented Nov 2, 2020

sandersn commented Feb 16, 2021

sandersn commented Mar 11, 2021

		return compareIdentifiers(s1.propertyName \|\| s1.name, s2.propertyName \|\| s2.name) \|\|
		compareIdentifiers(s1.name, s2.name);

Feat#Provide a quickfix for non-exported types #37980

Feat#Provide a quickfix for non-exported types #37980

Conversation

Qiyu8 commented Apr 15, 2020 • edited by DanielRosenwasser Loading

DanielRosenwasser left a comment

Choose a reason for hiding this comment

Qiyu8 commented Apr 28, 2020

sandersn left a comment

Choose a reason for hiding this comment

sandersn commented May 21, 2020

Qiyu8 commented May 22, 2020

Qiyu8 commented May 30, 2020

Qiyu8 commented Aug 4, 2020

DanielRosenwasser 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

DanielRosenwasser commented Aug 15, 2020

typescript-bot commented Aug 15, 2020 • edited Loading

typescript-bot commented Aug 15, 2020

Qiyu8 commented Aug 21, 2020

DanielRosenwasser commented Aug 21, 2020

typescript-bot commented Aug 21, 2020 • edited Loading

DanielRosenwasser commented Aug 21, 2020

typescript-bot commented Aug 21, 2020 • edited Loading

DanielRosenwasser left a comment • edited Loading

Choose a reason for hiding this comment

sandersn commented Oct 28, 2020

Qiyu8 commented Oct 30, 2020

sandersn commented Oct 30, 2020

Qiyu8 commented Nov 2, 2020

sandersn commented Feb 16, 2021

sandersn commented Mar 11, 2021

Qiyu8 commented Apr 15, 2020 •

edited by DanielRosenwasser

Loading

typescript-bot commented Aug 15, 2020 •

edited

Loading

typescript-bot commented Aug 21, 2020 •

edited

Loading

typescript-bot commented Aug 21, 2020 •

edited

Loading

DanielRosenwasser left a comment •

edited

Loading