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

Add fix for webpack history merge bug #29339

Merged
merged 3 commits into from
Mar 7, 2019

Conversation

weswigham
Copy link
Member

Fixes #28810, probably. I can't isolate a repro without webpack and a whole lot of other stuff (lodash, @type/histroy, react, more things that i'm not really sure about), but I've confirmed that this fixes @wanliyunyan's repro.

@weswigham weswigham requested a review from sandersn January 10, 2019 00:42
@wanliyunyan
Copy link

@weswigham thank you! After adding a non-empty check for symbol.valueDeclaration.parent, the error no longer occurs

@sheetalkamat
Copy link
Member

Wouldn't it mean valueDeclaration is sourceFile as thats the only one that shouldnt have parent or is it some kind of synthetic node? Did the debugging repro given not help with the isolation?

@weswigham
Copy link
Member Author

Yep, valueDeclaration is a source file. It's a source file merged with an interface caused by a UMD namespace declaration merge, the issue is with just those components I can't seem to make the issue repro. So I know the fix, and I know what caused the problem, but I can't write a test case that actually triggers the original bug... And I'm not really sure why.

@sheetalkamat
Copy link
Member

Could it be order of resolution and visible through webpack because it probably does not use sourceFile order to get errors? May be trying through editor kind of scenario where the errors are requested for the interface before sourceFile can find the issue?

@sandersn
Copy link
Member

@weswigham would it help to look at the call stack? I assume it's pretty easy to create a reference whose valueDeclaration has no parent and that the problem is that it normally doesn't make it into getTypeReferenceTypeWorker.

@sheetalkamat
Copy link
Member

@weswigham Found the repro for this while investigation of #29661 where ts-loader changes the order of files as the root files (history node typing before lib file) resulting in this error. The simplified repro is

//@filename: /other.d.ts
export as namespace SomeInterface;
export type Action = 'PUSH' | 'POP' | 'REPLACE';

//@filename: /main.ts
interface SomeInterface {
    readonly length: number;
}
const value: SomeInterface;

@weswigham
Copy link
Member Author

@sheetalkamat I've added your test to this PR.

@weswigham weswigham merged commit 4c9ad08 into microsoft:master Mar 7, 2019
@weswigham weswigham deleted the fix-webpacky-bug branch March 7, 2019 21:34
NejcZdovc added a commit to brave/brave-core that referenced this pull request Sep 19, 2019
This fixes history problem. More info here microsoft/TypeScript#28810.

It was fixed here microsoft/TypeScript#29339 with version 3.4.x
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TS 3.2.1 TypeError: Cannot read property 'kind' of undefined
4 participants