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.