Fix "isolatedDeclarations" QuickFix

[olmobrutall]

🔎 Search Terms

isolatedDeclarations quickfix TS9023

🕗 Version & Regression Information

This is an error in the new QuickFix in TS 5.5 to enable isolatedDeclarations.

⏯ Playground Link

No response

💻 Code

When activating isolatedDeclarations this code does not compile.

export function ImportExcelProgressModal(): React.JSX.Element {
  return <div></div>;
}

 //Error (active)	TS9023	Build:Assigning properties to functions without declaring them is not supported with --isolatedDeclarations. Add an explicit declaration for the properties assigned to this function.
ImportExcelProgressModal.show = (): Promise<boolean> => { 	
  return openModal(<ImportExcelProgressModal/>);
};

🙁 Actual behavior

After executing the quick fix it produces code with two errors:

export function ImportExcelProgressModal(): React.JSX.Element {
  return <div></div>;
}

export declare namespace ImportExcelProgressModal {
    //Error (active)	TS2300	(TS) Duplicate identifier 'show'.	
    export var show: () => Promise<bool>;
}


//Error (active)	TS2300	(TS) Duplicate identifier 'show'.	
ImportExcelProgressModal.show = (): Promise<boolean> => { 	
  return openModal(<ImportExcelProgressModal/>);
};

🙂 Expected behavior

but this (sorter) code would work:

export function ImportExcelProgressModal(): React.JSX.Element {
 return <div></div>;
}

export namespace ImportExcelProgressModal {
  export let show  = (): Promise<boolean> => {
      return openModal(<ImportExcelProgressModal/>);
  };
}

If ImportExcelProgressModal would have been exported as default then this would be the solution:

function ImportExcelProgressModal(): React.JSX.Element {
  return <div></div>;
}

namespace ImportExcelProgressModal {
  export let show  = (): Promise<boolean> => {
       return openModal(<ImportExcelProgressModal/>);
  };
}

export default ImportExcelProgressModal;

Additional information about the issue

No response

[andrewbranch]

It's not quite clear to me if the duplicate identifier error itself is a bug— assignments of initializers that aren't function expressions are allowed to merge with the existing sybol declaration during binding: https://www.typescriptlang.org/play/?noUnusedLocals=true&install-plugin=typescript-playground-link-shortener#code/CYUwxgNghgTiAEAzArgOzAFwJYHtVJxwAoBKALngDcctgBuAWAChRJYFUoBbEAZwAcoYBIkLwA3s3jSqseKmRcKUVAE9GTGbJhJUASVCpsiLCBjK1GrZTmJUAUQAe-OL165UF9cwC+zUTgAdApc8AC88ACMGijo2Hi6BiBGWCZmpBJ+TAGBdkkpaToReYbGpjAxhLkOzq7uCREZYQB8mRpAA

[sandersn]

After some discussion within the team, we decided that this is in fact a bug.

In JS, this code makes m a static method of f:

function f() { }
f.m = () => 1

Which is a little questionable since m isn't strictly speaking on the prototype the way most methods usually are. So it's not clear that it's worthwhile to mark it as such.

But it's definitely a bad idea in TS where you can have namespaces, because it results in a duplicate 'declaration':

function f() { }
declare namespace f {
  export function m(): number
}
f.m = () => 1

both export function m and f.m = end up counting as declarations.

The narrow fix is to avoid binding f.m = () => 1 as a method in TS.
The wider fix is, I think, to stop binding f.m = () => 1 as a method even in JS. f.prototype.m = () => 1 should continue to be bound as a method but I think it's fine to represent f.m as binding a property (whose type happens to be a function).

Both fixes boil down to deleting or conditionalising code in bindPotentiallyNewExpandoMemberToNamespace. The wider fix should make sure to continue binding prototype assignments as methods, so an additional parameter is probably needed if that's the way we want to go.

[sidlamsal]

Hi @sandersn , I've been working on this issue and wanted to ask for clarification on your comment before moving further.

To close this issue, the binding in bindPotentiallyNewExpandoMemberToNamespace() and the code fix fixMissingTypeAnnotationOnExports.ts need to be changed, correct?

The issue author wanted something like:

function f(): void { }
f.m = () => 1;

after the code fix to become something like:

function f(): void { }

namespace f {
    export var m = (): number => {
        return 1;
    };
}

And the binding change suggested in your comment would remove the Duplicate identifier 'm'. diagnostic from the code fix as it works now, something like:

function f(): void { }

declare namespace f {
    export var m: () => number;
}

f.m = () => 1;

Where currently, there is a Duplicate identifier 'm'. diagnostic on m.

Does that sound about right or have I misunderstood?

Thanks

[sandersn]

The codefix output is good enough as-is. In the following code (playground link), it works for n, just not for show:

export function ImportExcelProgressModal(): number {
  return 12
}

export declare namespace ImportExcelProgressModal {
    //Error (active)	TS2300	(TS) Duplicate identifier 'show'.	
    export var show: () => boolean;
    export var n: number
}


//Error (active)	TS2300	(TS) Duplicate identifier 'show'.	
ImportExcelProgressModal.show = (): boolean => { 	
  return false;
};
ImportExcelProgressModal.n = 13

You just need to change the binder to treat function initialisers the same as other types. Right now they're treated as if they're a new method on ImportExcelProgressModal instead just a property.

This is how Corsa works already, I believe, so Strada should change to match -- this is the wider fix I was talking about above.