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

Use built local on CI and not LKG #29886

Merged
merged 4 commits into from
Feb 13, 2019

Conversation

weswigham
Copy link
Member

Should prevent the nightly publish from failing again from us being unable to built ourselves with the current snapshot again~

@weswigham
Copy link
Member Author

Note: This PR should fail until I add another commit to fix the build break.

@weswigham
Copy link
Member Author

weswigham commented Feb 13, 2019

Okay, and that should fix our build.

@ahejlsberg should we look into why

    export function assign<T extends object>(t: T, ...args: (T | undefined)[]) {
        for (const arg of args) {
            for (const p in arg!) {
                if (hasProperty(arg!, p)) {
                    t![p] = arg![p]; // TODO: GH#23368
                }
            }
        }
        return t;
    }

no longer typechecks (or should we call this acceptable)? I'd assume the cause if one of #29847, #29787, or #29858 - I'd wager the last, given this is a function with a rest parameter. It seems like we're wrapping the type with an excess NonNullable alias which, since they don't compose and simplify, makes the assignment fail. It may equally be worth making getGlobalNonNullableTypeInstantiation not wrap something which is already an instance of NonNullable with another layer of it to avoid the issue for now.

if (arg === undefined) continue;
for (const p in arg) {
if (hasProperty(arg, p)) {
t[p] = arg[p]; // TODO: GH#23368
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the TODO anymore?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mm the comment isn't relevant, but the issue still stands.

@weswigham weswigham merged commit 6d2b738 into microsoft:master Feb 13, 2019
@ahejlsberg
Copy link
Member

@weswigham I suspect the build break was caused by #27697, but a contributing factor appears to be some code that was added in #28348 to add non-nullability to the right hand expression in a for-in statement. That code behaves differently than a corresponding narrowing comparison with null or undefined which is why you could fix the issue by adding an if statement.

@weswigham
Copy link
Member Author

Mmm very possibly, we don't publish nightlies on weekends, but I forgot to include the things merged over the weekend in the candidates for the failure. In any case, I believe #29437 plus the enhancements to empty intersection calculations with type parameters in the negated types PR would fix the issue (as NonNullable<T> would simplify to T as T extends object).

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.

3 participants