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

Emit of union type metadata in nullStrictMode is always Object #12703

Closed
dqweek opened this issue Dec 7, 2016 · 7 comments
Closed

Emit of union type metadata in nullStrictMode is always Object #12703

dqweek opened this issue Dec 7, 2016 · 7 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@dqweek
Copy link

dqweek commented Dec 7, 2016

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

function annotation(): PropertyDecorator {
    return (target: any, key: string | symbol): void => { };
}

class A {

    @annotation()
    field: B | null | undefined;
}

class B {
    field1: number;
}

Expected behavior:


function annotation() {
    return function (target, key) { };
}
var A = (function () {
    function A() {
    }
    __decorate([
        annotation(), 
        __metadata('design:type', B)
    ], A.prototype, "field", void 0);
    return A;
}());
var B = (function () {
    function B() {
    }
    return B;
}());

Actual behavior:


function annotation() {
    return function (target, key) { };
}
var A = (function () {
    function A() {
    }
    __decorate([
        annotation(), 
        __metadata('design:type', Object) //<-- should be B
    ], A.prototype, "field", void 0);
    return A;
}());
var B = (function () {
    function B() {
    }
    return B;
}());

@dqweek
Copy link
Author

dqweek commented Dec 7, 2016

believed to be related to issues #11391, #10809

@mhegazy
Copy link
Contributor

mhegazy commented Dec 7, 2016

Union types are emitted as Object since there is no way to express the union using runtime values.

@mhegazy mhegazy added the Working as Intended The behavior described is the intended behavior; this is not a bug label Dec 7, 2016
@dqweek
Copy link
Author

dqweek commented Dec 7, 2016

I disagree. I don't argue that all union types should behave like I have mentioned, but only when single type unioned with null or undefined or both.
I see this union as special case, cause the only type you should to emit is the B type, null and undefined by them self are not types, but definition for missing states (so they don't need to be reflected in meta in any way)

if you still think it is working as intended. please mark this future as Breaking change

Till the use of strictNull I could do the following:

class A {}

class B
{
   @dec()
   field: A
}

const variable: B;
variable.field = null;

and the code would compile as expected and field type would be emitted as expected allowing me at Runtime to expect it and use it.

now, when the strict flag is on, all fields that can accept null or undefined, emitted as Object type, which become the metadata unusable in my case and I guess for others who relied on this behavior

@mhegazy
Copy link
Contributor

mhegazy commented Dec 7, 2016

if you are not using --strictNullChecks, both null and undefined are in the domain of all types. so you could assign null to any variable with no errors. with --strictNullChecks, this becomes more strict, and you need to mark ones that accept null or undefined

@dqweek
Copy link
Author

dqweek commented Dec 7, 2016

I understand that,
I want to enjoy the strict mode future and it great code analysis, but still without loosing all benefits of the less strict mode that I was using before.

the null | undefined addition was seemed straight forward change, but now cause of the union definition, the metadata loosing it purpose, and it breaking my code that relies on this emit.

I looking a way to enjoy both worlds :-)

@mhegazy
Copy link
Contributor

mhegazy commented Dec 7, 2016

why is Foo | null any different from Foo | number?

@dqweek
Copy link
Author

dqweek commented Dec 8, 2016

As I see it,

Foo | number union is splitted to two different valid types in javascript world. number is primitive type and Foo is descendant of Object type.

Foo | null union is a more special case. if we try to split it, we will find only Foo type and null hint for compiler, tells that field defined with that union can have valid Foo type or being null reference. from JavaScript perspective, null is not a Object descendant or primitive type.

this definition could be the same:

let defined: @null() decorator that hints compiler, field can have null reference
let defined: @undefined() decorator that hints compiler, field can have undefined state

then in theory this definition should be equal:

class A
{
     field: Foo | null | undefined;
}

class B
{
    @Null()
    @Undefined()
    field: Foo;
}

on both cases the compiler will understand the hint for allowing null | undefined definitions and code analysis would work, but in A type the union will emit Object type meta, in B the emit will be B type

for my opinion, the null, undefined tokens should be ignored only on metadata emit, they should still continue to participate in all other aspects for the union analysis logic

@mhegazy mhegazy closed this as completed Apr 21, 2017
nitzantomer added a commit to kinecosystem/marketplace-server that referenced this issue Mar 25, 2018
turns out that when using union types with `undefined` the compiler will save the type as `Object` (more info: microsoft/TypeScript#12703), which isn't good for typeorm.
Instead, mark nullable props as optional.
@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
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

2 participants