fix: handle deeply nested whole exports #111

FredericEspiau · 2024-12-19T12:58:17Z

Fixes #109

But if you have a loop in your whole exports, now you are doomed 😬

FredericEspiau · 2024-12-19T12:59:07Z

lib/util/edit.ts

@@ -92,7 +93,7 @@ const createLanguageService = ({
  projectRoot: string;
  fileService: FileService;
 }) => {
-  const languageService = ts.createLanguageService({
+  return ts.createLanguageService({


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

FredericEspiau · 2024-12-19T12:59:26Z

lib/util/edit.ts

@@ -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] || '';


\s is the same as [\s]

FredericEspiau · 2024-12-19T13:00:22Z

lib/util/edit.ts

@@ -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';


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

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 😎

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

FredericEspiau · 2024-12-19T13:03:00Z

lib/util/edit.ts

@@ -424,23 +475,19 @@ const processFile = ({
            break;
          }
          case 'whole': {
-            if (!item.file) {
+            if (!Export.WholeExportDeclaration.isFileFound(item)) {


I had to create a type guard because TypeScript couldn't determine that item.file couldn't be undefined in my deeplyGetExportNames function

FredericEspiau · 2024-12-19T13:04:17Z

lib/util/findFileUsage.ts

@@ -2,7 +2,8 @@ import ts from 'typescript';
 import { Vertexes } from './DependencyGraph.js';
 import { parseFile } from './parseFile.js';

-const ALL_EXPORTS_OF_UNKNOWN_FILE = '__all_exports_of_unknown_file__';
+const ALL_EXPORTS_OF_UNKNOWN_FILE = '#all_exports_of_unknown_file#';
+const CIRCULAR_DEPENDENCY = '#circular_dependency#';


This doesn't the whole circular import issue though

FredericEspiau · 2024-12-19T13:04:38Z

lib/util/parseFile.ts

@@ -128,143 +129,6 @@ const getChange = (
  };
 };

-type Export =


Refactored in their own files

kazushisan

Thank you for your awesome work 🚀

kazushisan · 2024-12-19T14:05:26Z

lib/util/edit.ts

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

  if (changes.length === 0) {
-    const result = {
+    return {


actually, I created the result const intentionally so that I get the correct type inference without having to annotate the return type of the function. Would you mind reverting this?

https://www.typescriptlang.org/play/?#code/PTAEBcAsFNQMwPYBskIO4EsB2BzCBPAB1mzmgCdosBjEgZ1CwXFDUgEMW1Y12twAUCHgBXGuAwIs8ABQBKAFygA3kLCgNBYkoBE7HQG41mjQA8ldcOWw4jwk-gD8SsQBNocbNFd2wAX1AAHxVjE3AiaF0AI0NQzXwLKxtfE1BTZ1A3Dy8fYz8BODFqCSlZORCNDDhQGQBZTkgAOnI+VwQAW3lQAD5QRoBWctUTSnARcmlh1PDtUABydjnQdgZqKUsAGgFUtKU5xAQ5rZN8jXzt0FHxyYuNGcj5qKWV0DWsTdvQBMf2ciOL-LnYzcVh8FhQDAMe4KYyFcSSaQ4eRKVT2TTQ0B6WJosyJay4FIBYKo9ToiLRbGkjTfSz42x5ApFEqIrpTSrVOoNZqtDpdXoDIafN6WS7QOgiJAsAC8FWm5Pmi2Wq3W4GOqXM8wO-xOAk+VwmovFkqMZ11GmFLEoRulsru8rmTyVrxVaupeyiv21oHOGn10itEvARj8QA

kazushisan · 2024-12-19T14:07:57Z

lib/util/export/wholeExportDeclaration.ts

+/**
+ * Whole export when the file is found within the destFiles
+ */
+export type FileFound = {


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

export type FileFound = WholeExportDeclaration & { file: string }

kazushisan · 2024-12-19T14:09:15Z

lib/util/findFileUsage.ts

@@ -2,7 +2,8 @@ import ts from 'typescript';
 import { Vertexes } from './DependencyGraph.js';
 import { parseFile } from './parseFile.js';

-const ALL_EXPORTS_OF_UNKNOWN_FILE = '__all_exports_of_unknown_file__';
+const ALL_EXPORTS_OF_UNKNOWN_FILE = '#all_exports_of_unknown_file#';


I like your idea of using # for special keywords. Definitely better than my original implementation!

kazushisan · 2024-12-19T14:11:53Z

lib/util/edit.ts

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


Suggested change

*

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

I think this comment is too much [nit]

kazushisan · 2024-12-19T14:16:33Z

lib/util/export.ts

@@ -0,0 +1,16 @@
+import ts from 'typescript';


This is just a matter of preference, but I like to avoid splitting files until its absolutely necessary. I think the export.ts file is okay but can you move the code of namedExports.ts and wholeExportDeclaration.ts into this single file to align with the other parts of the project?

kazushisan · 2024-12-19T14:23:48Z

lib/util/edit.ts

+  files,
+  fileNames,
+  options,
+  filesAlreadyVisited = new Set<string>(),


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)

kazushisan · 2024-12-19T14:26:20Z

lib/util/edit.test.ts

@@ -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', () => {


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?

kazushisan · 2024-12-19T14:28:34Z

lib/util/edit.ts

@@ -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';


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 😎

FredericEspiau commented Dec 19, 2024

View reviewed changes

fix: handle deeply nested whole exports

1e5eda1

FredericEspiau force-pushed the fix/handle-deeply-nested-whole-exports branch from dbdf754 to 1e5eda1 Compare December 19, 2024 13:01

FredericEspiau commented Dec 19, 2024

View reviewed changes

kazushisan reviewed Dec 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: handle deeply nested whole exports #111

fix: handle deeply nested whole exports #111

FredericEspiau commented Dec 19, 2024 •

edited

Loading

FredericEspiau Dec 19, 2024

FredericEspiau Dec 19, 2024

FredericEspiau Dec 19, 2024

kazushisan Dec 19, 2024

kazushisan Dec 20, 2024 •

edited

Loading

FredericEspiau Dec 19, 2024

FredericEspiau Dec 19, 2024

FredericEspiau Dec 19, 2024

kazushisan left a comment

kazushisan Dec 19, 2024

kazushisan Dec 19, 2024

kazushisan Dec 19, 2024

kazushisan Dec 19, 2024

kazushisan Dec 19, 2024

kazushisan Dec 19, 2024

kazushisan Dec 19, 2024

kazushisan Dec 19, 2024

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

fix: handle deeply nested whole exports #111

Are you sure you want to change the base?

fix: handle deeply nested whole exports #111

Conversation

FredericEspiau commented Dec 19, 2024 • edited Loading

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

kazushisan Dec 20, 2024 • edited Loading

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

kazushisan 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

FredericEspiau commented Dec 19, 2024 •

edited

Loading

kazushisan Dec 20, 2024 •

edited

Loading