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

fix: handle deeply nested whole exports #111

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions lib/util/edit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1130,6 +1130,26 @@ export const b = 'b';`,
assert.equal(fileService.exists('/app/a_reexport.ts'), false);
assert.equal(fileService.exists('/app/a.ts'), false);
});

it('should look for deeply nested whole re-export without removing files', () => {
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 the logic not supporting loops is acceptable as long as the behavior is defined 👍 Can you add a test case to assure what happens when there's a loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well your test case will just hang until you stop it, so there isn't much I can do about this

const fileService = new MemoryFileService();
fileService.set('/app/main.ts', `import { c } from './a';`);
fileService.set('/app/a.ts', `export * from './b';`);
fileService.set('/app/b.ts', `export * from './c';`);
fileService.set('/app/c.ts', `export const c = 'c';`);

edit({
fileService,
recursive,
deleteUnusedFile: true,
entrypoints: ['/app/main.ts'],
});

assert.equal(fileService.get('/app/main.ts'), `import { c } from './a';`);
assert.equal(fileService.get('/app/a.ts'), `export * from './b';`);
assert.equal(fileService.get('/app/b.ts'), `export * from './c';`);
assert.equal(fileService.get('/app/c.ts'), `export const c = 'c';`);
});
});

describe('namespace export declaration', () => {
Expand Down
83 changes: 63 additions & 20 deletions lib/util/edit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { MemoryFileService } from './MemoryFileService.js';
import { findFileUsage } from './findFileUsage.js';
import { parseFile } from './parseFile.js';
import { Output } from './Output.js';
import * as Export from './export.js';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've used namespaces to organise the code. This way you can do stuff like

Export.DeclarationWholeExport.isFileFound

Tell me what you think about this

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 have any particular opinion about using namespace imports (besides, it's not an issue in terms of detecting unused code since tsr can detect these 😎

Copy link
Contributor

@kazushisan kazushisan Dec 20, 2024

Choose a reason for hiding this comment

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

I’ve been thinking about this, but can we use import { foo } from 'foo' to keep the style consistent? Sorry for the confusion🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!


const transform = (
source: string,
Expand Down Expand Up @@ -92,7 +93,7 @@ const createLanguageService = ({
projectRoot: string;
fileService: FileService;
}) => {
const languageService = ts.createLanguageService({
return ts.createLanguageService({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tell me if you want me to revert these kind of changes, I've got some OCDs about this kind of stuffs 😬

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted

getCompilationSettings() {
return options;
},
Expand All @@ -118,8 +119,6 @@ const createLanguageService = ({
return fileService.get(name);
},
});

return languageService;
};

const updateExportDeclaration = (code: string, unused: string[]) => {
Expand Down Expand Up @@ -148,7 +147,7 @@ const updateExportDeclaration = (code: string, unused: string[]) => {

const printer = ts.createPrinter();
const printed = result ? printer.printFile(result).replace(/\n$/, '') : '';
const leading = code.match(/^([\s]+)/)?.[0] || '';
const leading = code.match(/^(\s+)/)?.[0] || '';
FredericEspiau marked this conversation as resolved.
Show resolved Hide resolved

return `${leading}${printed}`;
};
Expand Down Expand Up @@ -181,6 +180,58 @@ const getSpecifierPosition = (exportDeclaration: string) => {
return result;
};

/**
* Retrieves the names of the exports from a whole export declaration.
* For each whole export declaration, it will recursively get the names of the exports from the file it points to.
*
* No need to memoize this function because `parseFile` already memoizes the file parsing.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
*
* No need to memoize this function because `parseFile` already memoizes the file parsing.

I think this comment is too much [nit]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

*/
const deeplyGetExportNames = ({
item,
files,
fileNames,
options,
filesAlreadyVisited = new Set<string>(),
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 that this field is exposing unnecessary choice/information outside of the function's boundary. Can this be declared inside the function?

function foo() {
 const visited = new Set();

  // do something then
  deeply(theOtherArgs, visited)
}

or maybe using a stack instead of recursively calling functions is better to have a shared context. (converting to using a stack is just a suggestion, not a must)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, I've kept the recursion because I didn't know how to do it with the stack 😄

}: {
item: Export.WholeExportDeclaration.FileFound;
files: Map<string, string>;
fileNames: Set<string>;
options: ts.CompilerOptions;
filesAlreadyVisited?: Set<string>;
}): string[] => {
if (filesAlreadyVisited.has(item.file)) {
return [];
}

const parsed = parseFile({
file: item.file,
content: files.get(item.file) || '',
options,
destFiles: fileNames,
});

const deepExportNames = parsed.exports
.filter(
(v) =>
Export.isWholeExportDeclaration(v) &&
Export.WholeExportDeclaration.isFileFound(v),
)
.flatMap((v) =>
deeplyGetExportNames({
item: v,
files,
fileNames,
options,
filesAlreadyVisited: filesAlreadyVisited.add(item.file),
}),
);

return parsed.exports
.filter(Export.isNamedExport)
.flatMap((v) => v.name)
.concat(deepExportNames);
};

const processFile = ({
targetFile,
files,
Expand Down Expand Up @@ -424,23 +475,19 @@ const processFile = ({
break;
}
case 'whole': {
if (!item.file) {
if (!Export.WholeExportDeclaration.isFileFound(item)) {
FredericEspiau marked this conversation as resolved.
Show resolved Hide resolved
// whole export is directed towards a file that is not in the project
break;
}

const parsed = parseFile({
file: item.file,
content: files.get(item.file) || '',
const exportNames = deeplyGetExportNames({
item,
files,
fileNames,
options,
destFiles: fileNames,
});

const exported = parsed.exports.flatMap((v) =>
'name' in v ? v.name : [],
);

if (exported.some((v) => usage.has(v))) {
if (exportNames.some((v) => usage.has(v))) {
break;
}

Expand Down Expand Up @@ -536,13 +583,11 @@ export {};\n`,
}

if (changes.length === 0) {
const result = {
return {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's your project mate, ask me and I'll do it 😄. This is done.

operation: 'edit' as const,
content: files.get(targetFile) || '',
removedExports: logs,
};

return result;
}

let content = applyTextChanges(files.get(targetFile) || '', changes);
Expand Down Expand Up @@ -582,13 +627,11 @@ export {};\n`,

fileService.set(targetFile, content);

const result = {
return {
operation: 'edit' as const,
content: fileService.get(targetFile),
removedExports: logs,
};

return result;
};

export const edit = ({
Expand Down
16 changes: 16 additions & 0 deletions lib/util/export.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import ts from 'typescript';
kazushisan marked this conversation as resolved.
Show resolved Hide resolved

import { NamedExport } from './export/namedExport.js';
import { WholeExportDeclaration } from './export/wholeExportDeclaration.js';

export * as NamedExport from './export/namedExport.js';
export * as WholeExportDeclaration from './export/wholeExportDeclaration.js';

export type Export = NamedExport | WholeExportDeclaration;

export const isNamedExport = (v: Export): v is NamedExport => 'name' in v;

export const isWholeExportDeclaration = (
v: Export,
): v is WholeExportDeclaration =>
v.kind === ts.SyntaxKind.ExportDeclaration && v.type === 'whole';
141 changes: 141 additions & 0 deletions lib/util/export/namedExport.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
import ts from 'typescript';

export type ClassDeclaration = {
kind: ts.SyntaxKind.ClassDeclaration;
name: string;
change: {
code: string;
isUnnamedDefaultExport?: boolean;
span: {
start: number;
length: number;
};
};
skip: boolean;
start: number;
};

export type EnumDeclaration = {
kind: ts.SyntaxKind.EnumDeclaration;
name: string;
change: {
code: string;
span: {
start: number;
length: number;
};
};
skip: boolean;
start: number;
};

export type ExportAssignment = {
kind: ts.SyntaxKind.ExportAssignment;
name: 'default';
change: {
code: string;
span: {
start: number;
length: number;
};
};
skip: boolean;
start: number;
};

export type FunctionDeclaration = {
kind: ts.SyntaxKind.FunctionDeclaration;
name: string;
change: {
code: string;
isUnnamedDefaultExport?: boolean;
span: {
start: number;
length: number;
};
};
skip: boolean;
start: number;
};

export type InterfaceDeclaration = {
kind: ts.SyntaxKind.InterfaceDeclaration;
name: string;
change: {
code: string;
span: {
start: number;
length: number;
};
};
skip: boolean;
start: number;
};

export type NameExportDeclaration = {
kind: ts.SyntaxKind.ExportDeclaration;
type: 'named';
name: string[];
skip: boolean;
change: {
code: string;
span: {
start: number;
length: number;
};
};
start: number;
};

export type NamespaceExportDeclaration = {
kind: ts.SyntaxKind.ExportDeclaration;
type: 'namespace';
name: string;
start: number;
change: {
code: string;
span: {
start: number;
length: number;
};
};
};

export type TypeAliasDeclaration = {
kind: ts.SyntaxKind.TypeAliasDeclaration;
name: string;
change: {
code: string;
span: {
start: number;
length: number;
};
};
skip: boolean;
start: number;
};

export type VariableStatement = {
kind: ts.SyntaxKind.VariableStatement;
name: string[];
change: {
code: string;
span: {
start: number;
length: number;
};
};
skip: boolean;
start: number;
};

export type NamedExport =
| ClassDeclaration
| EnumDeclaration
| ExportAssignment
| FunctionDeclaration
| InterfaceDeclaration
| NameExportDeclaration
| NamespaceExportDeclaration
| TypeAliasDeclaration
| VariableStatement;
43 changes: 43 additions & 0 deletions lib/util/export/wholeExportDeclaration.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
import ts from 'typescript';

/**
* Whole export when the file is found within the destFiles
*/
export type FileFound = {
Copy link
Contributor

Choose a reason for hiding this comment

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

declaring a type which is almost the same seems a bit redundant. How about

export type FileFound = WholeExportDeclaration & { file: string }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this kind of case what I usually is

type Base = { ... };
type SubCase1 = Base & { ... };
type SubCase2 = Base & { ... };
type Whole = SubCase1 | SubCase2;

Because Base isn't actually a type you can use as it's not really a WholeExportDeclaration, which has either file: string or file: null, WDYT?

kind: ts.SyntaxKind.ExportDeclaration;
type: 'whole';
file: string;
specifier: string;
start: number;
change: {
code: string;
span: {
start: number;
length: number;
};
};
};

/**
* Whole export when the file is not found within the destFiles, i.e. the file is not part of the project
*/
export type FileNotFound = {
kind: ts.SyntaxKind.ExportDeclaration;
type: 'whole';
file: null;
specifier: string;
start: number;
change: {
code: string;
span: {
start: number;
length: number;
};
};
};

export type WholeExportDeclaration = FileFound | FileNotFound;

export const isFileFound = (
exportDeclaration: WholeExportDeclaration,
): exportDeclaration is FileFound => exportDeclaration.file !== null;
Loading
Loading