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

Flow Analyze don't recognize combined Intersection and Union Type #12345

Closed
k8w opened this issue Nov 18, 2016 · 14 comments
Closed

Flow Analyze don't recognize combined Intersection and Union Type #12345

k8w opened this issue Nov 18, 2016 · 14 comments

Comments

@k8w
Copy link

k8w commented Nov 18, 2016

TypeScript Version: 2.1.1 / nightly (2.2.0-dev.201xxxxx)
Both 2.0.10 and 2.1.1

Problem
Flow Analyze don't work as expected when combining Intersection and Union syntax.

Example

B&A B&C works well as Code1.
A|C works well as Code2.
B&A | B&C compiled error as Code3.

test2.ts(18,21): error TS2339: Property 'a' does not exist on type '(B & A) | (B & C)'.
Property 'a' does not exist on type 'B & C'.

So should this be working as intended?

(B is to change type to required, as Code1, this works.)

Def A,B,C

interface A {
    type?: 'a';
    a?: string;
}

interface B{
    type: 'a' | 'c';
}

interface C{
    type?: 'c';
    c?: string;
}

Code 1: B&A B&C works well

//change type to required, works well
var ba: B&A;
if(ba.type == 'a'){
    console.log(ba.a)
}

//change type to required, works well
var bc: B&C;
if(bc.type == 'c'){
    console.log(bc.c)
}

Code 2: A|C works well

//works well
var ac: A|C;
if(ac.type == 'a'){
    console.log(ac.a)
}

Code 3: B&A | B&C not works

//Compiled error
var abc: B&A | B&C;
if(abc.type == 'a'){
    console.log(abc.a)
}
@HerringtonDarkholme
Copy link
Contributor

HerringtonDarkholme commented Nov 18, 2016

This works as intended.
You should not use the intersection syntax, or BaseCharSeries should not have type field.
The intersection type has conflicting string type that invalidates flow analyze.

You can use interface extends instead, or remove type field.

@kitsonk
Copy link
Contributor

kitsonk commented Nov 18, 2016

Just to note that removal of the type from BaseChartSeries still does not narrow.

@HerringtonDarkholme
Copy link
Contributor

@kitsonk It works in nightly due to #11717

@k8w
Copy link
Author

k8w commented Nov 21, 2016

@HerringtonDarkholme Right, it works after removing type from BaseChartSeries.

But what if I need to ensure all extended XXXSeries to contain field type ?

@HerringtonDarkholme
Copy link
Contributor

You can use interface extends instead

interface BaseChartSeries {
    name: string;
    type: string;
}

//Line
interface LineSeriesSettings extends BaseChartSeries {
    type: 'line';
    lineColor?: string;
}

@k8w
Copy link
Author

k8w commented Nov 21, 2016

@HerringtonDarkholme
My actual scene is below with the same problem.

BaseSeriesSettings is base class for series settings, may include some common property, every field may be optional, because it may be a small Object only for override some field.
BaseSeries is base class extends BaseSeriesSettings , with some fields (from XxSeriesSettings) to be marked as required (e.g. type) , and some other common property (e.g. name) .
ChartSeries is just like a versatile class with automatically type check to use.

ChartSeries may be final thing transfered to a Chart component, and a SeriesSettings may be a optional Object to override something (so type and some other fields be optionnal) .

But it compiled Error.

test.ts(32,17): error TS2339: Property 'lineColor' does not exist on type 'ChartSeries'.
Property 'lineColor' does not exist on type 'BarSeries'.

So why there is error when nested extend relationship exist?

//Base Class for ChartSeries
interface BaseChartSeries {
    name: string;
    type: string;
}

interface BaseSeriesSettings{
    type: string;
    commonProp1: string;
    commonProp2: number;
}

//Line
interface LineSeriesSettings extends BaseSeriesSettings{
    type: 'line';
    lineColor?: string;
}
type LineSeries = BaseChartSeries & LineSeriesSettings;

//Bar
interface BarSeriesSettings  extends BaseSeriesSettings{
    type: 'bar';
}
type BarSeries = BaseChartSeries & BarSeriesSettings;

//Export ChartSeries
type ChartSeries = LineSeries | BarSeries;

//Test Code (Compile Error)
var series:ChartSeries;
if (series.type == 'line') {
    var a = series.lineColor;
}

@HerringtonDarkholme
Copy link
Contributor

@twoeo Github issues isn't for Q&A. Would you mind putting this in stackoverflow? I'm happy to answer it.

Your original issues isn't a bug but rather a question. So I think we can close discussion here and move to a better place.

@k8w
Copy link
Author

k8w commented Nov 21, 2016

@HerringtonDarkholme
Well.. Never regard this as Q&A, maybe I should change a way to describe.

Main problem is:
Flow Analyze don't work as expected when combining Intersection and Union syntax.

Example

B&A B&C works well as Code1.
A|C works well as Code2.
B&A | B&C compiled error as Code3.

test2.ts(18,21): error TS2339: Property 'a' does not exist on type '(B & A) | (B & C)'.
Property 'a' does not exist on type 'B & C'.

So should this be working as intended?

(B is to change type to required, as Code1, this works.)

Def A,B,C

interface A {
    type?: 'a';
    a?: string;
}

interface B{
    type: 'a' | 'c';
}

interface C{
    type?: 'c';
    c?: string;
}

Code 1: B&A B&C works well

//change type to required, works well
var ba: B&A;
if(ba.type == 'a'){
    console.log(ba.a)
}

//change type to required, works well
var bc: B&C;
if(bc.type == 'c'){
    console.log(bc.c)
}

Code 2: A|C works well

//works well
var ac: A|C;
if(ac.type == 'a'){
    console.log(ac.a)
}

Code 3: B&A | B&C not works

//Compiled error
var abc: B&A | B&C;
if(abc.type == 'a'){
    console.log(abc.a)
}

@k8w k8w changed the title Flow Analyze don't recognize nested type&interface Flow Analyze don't recognize combined Intersection and Union Type Nov 21, 2016
@gcnew
Copy link
Contributor

gcnew commented Nov 21, 2016

You initial example code works in latest if you remove type: string from BaseChartSeries. If you are suggesting simplification of string & 'literal' to just 'literal' it's already been suggested in #12289.

The latest example seems to be a special case of simplification with literal union. On a side note: such use of optional discrimination properties looks vague and rather confusing.

@k8w
Copy link
Author

k8w commented Nov 21, 2016

Thanks @gcnew ~
But removing type: string is different with my intention.
As type in B (lastest code), the purpose is to keep 2 same interface, which have different requirement of fields.

@k8w
Copy link
Author

k8w commented Nov 21, 2016

It may not be vague in a specific scene.
like ChartSeries and SeriesSettings.

Another example may be React.
Every React component have a State interface.
You need provide every required field in initialization of a State,
but don't need to do so when setState({xxx:xxx})

@HerringtonDarkholme
Copy link
Contributor

HerringtonDarkholme commented Nov 21, 2016

//Base Class for ChartSeries
interface BaseChartSeries {
    name: string;
    type: string;
}

interface BaseSeriesSettings{
    type: string;
    commonProp1: string;
    commonProp2: number;
}

//Line
interface LineSeriesSettings extends BaseSeriesSettings, BaseChartSeries{
    type: 'line';
    lineColor?: string;
}
type LineSeries =  LineSeriesSettings;

//Bar
interface BarSeriesSettings  extends BaseSeriesSettings, BaseChartSeries {
    type: 'bar';
}
type BarSeries =  BarSeriesSettings;

//Export ChartSeries
type ChartSeries = LineSeries | BarSeries;

//Test Code (Compile Error)
var series:ChartSeries;
if (series.type == 'line') {
    var a = series.lineColor;
}

If you still need LineSeriesSettings without BaseChartSeries, declare new interface.

@gcnew
Copy link
Contributor

gcnew commented Nov 21, 2016

Maybe the new Mapped Types and Partial<T> can help with the optionalization.

@k8w
Copy link
Author

k8w commented Nov 21, 2016

Thanks @HerringtonDarkholme , it works and useful.
Thanks @gcnew , this may be best solution, waiting for 2.2.

@k8w k8w closed this as completed Nov 21, 2016
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants