Fixed crash in import fixes for augmented modules #58965
Conversation
typescript-bot
added
the
For Milestone Bug
Andarist
force-pushed
the
fix/import-generation-crash-augmented-modules
branch
from
86c3e94 to
7567a31
Compare
|
@typescript-bot test it |
|
Hey @iisaduan, the results of running the DT tests are ready. Everything looks the same! |
|
@iisaduan Here are the results of running the user tests with tsc comparing Everything looks good! |
|
@iisaduan Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@iisaduan Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
|
The new error might be a positive change but I'm not entirely sure. Module augmentation is a weird beast. Test case: // @strict: true
// @module: nodenext
// @moduleResolution: nodenext
// @noEmit: true
// @skipLibCheck: true
// @filename: node_modules/urql/index.d.ts
type AnyVariables =
| {
[prop: string]: any;
}
| void
| undefined;
declare function useQuery<Data = any, Variables extends AnyVariables = AnyVariables>(
args: Variables
): Data;
export { AnyVariables, useQuery };
// @filename: src/index.ts
import { AnyVariables, useQuery } from 'urql';
declare module 'urql' {
export function useQuery<Data = any, Variables extends AnyVariables = AnyVariables>(
name: string,
args: Variables
): Data;
}The important ingredient here is That said, I'm probably somewhat surprised that this is an error at all. I guess it's different to export a declaration using a modifier or using a separate export declaration but... it really feels like a detail that the user shouldn't be concerned with. I'm also not sure how to even augment this correctly since this is not allowed |
I agree that it seems like a detail that users shouldn't need to worry about, since without the module augmentation, they should be the same, and it seems we allow |
Thanks. I just stopped reducing at the point that test could be easily debugged and proceeded to fixing it 😅 I added this now as an extra test case here.
That's likely related to the directly exported |
|
Is there any progress on this? Like, a dev-release that can be installed through NPM with a fix, or maybe a workaround? |
Once this or another bug fix PR is merged, the nightly build of that day will contain the fix. |
This new error seems "correct" in that those two |
There was a problem hiding this comment.
Isabel and I are investigating this, but I just wanted to block this from merging for now as it’s not correct. In Isabel’s test case, under this PR, we can see the merged Container interface symbol has a parent of a non-merged external module symbol, when (if anything), the parent of the merged export should be the merged module. In general, when merging two symbols, we don’t want to just arbitrarily pick the parent of one of the constituents to be the sole parent of the new symbol, which is what this PR is doing.
There was a problem hiding this comment.
I don't think this is the correct fix, for a couple reasons:
- If you alias the exported symbol that will be augmented (
Containerin this case), the crash still happens - The parent that is being assigned here is incorrect, as the merged symbol is not a descendent of the unmerged module/augmentation
I spent some time with @andrewbranch (oops now I just saw your comment) testing the module augmentation behavior and it does work correctly with the merged symbols not having parents. As a result, I don't think it makes sense to generate parents for all symbols of this type if they're not needed in typechecking.
I will open another PR that fixes autoimports directly. (#59582)
| //// | ||
| //// declare class AliasPiece extends Piece {} | ||
| //// | ||
| //// export { AliasPiece, type Container }; |
There was a problem hiding this comment.
| //// export { AliasPiece, type Container }; | |
| //// export { AliasPiece, type Container as C}; |
If you then try to augment an aliased export, the crash still happens
|
superseded by #59582 |
fixes #58907