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

Invalid TS2339 Property '<prop>' does not exist on type 'never' in if/else branch #21517

Closed
Sebazzz opened this issue Jan 31, 2018 · 9 comments
Closed
Labels
Breaking Change Would introduce errors in existing code

Comments

@Sebazzz
Copy link

Sebazzz commented Jan 31, 2018

I think I may have found an issue in the newly released Typescript 2.7 release. I did not have this issue with Typescript 2.6.

TypeScript Version: 2.7.0

Search Terms:
TS2339
'never'
'classlist'

Code

import * as ko from 'knockout';

ko.bindingHandlers.validationProperty = {
    init(element: HTMLInputElement, valueAccessor: () => string, allBindingsAccessor: KnockoutAllBindingsAccessor, viewModel: any, bindingContext: KnockoutBindingContext) {
        const validatable = {modelState:()=>{return {} as any;}},
            property = valueAccessor();

        if (!validatable) {
            return;
        }

        function mark(validationState: string|undefined) {
            if ('setCustomValidity' in element) {
                element.setCustomValidity(validationState ? validationState : '');

                if (!validationState) {
                    element.removeAttribute('required');
                } else {
                    element.setAttribute('required', 'required');
                }
            } else {
                if (validationState) {
                    element.classList.add('is-invalid');
                } else {
                    element.classList.remove('is-invalid');
                }
            }
        }

        ko.computed(() => {
            const modelState = validatable.modelState();
            if (!modelState) {
                return;
            }

            const propertyState = modelState[property];
            if (!propertyState) {
                mark(undefined);
            } else {
                mark(propertyState.join('\r\n'));
            }
        }).extend({ disposeWhenNodeIsRemoved: element });
    }
};
   tsc --noEmit --pretty ./js/AppFramework/Forms/ValidationMessage.ts
js/AppFramework/Forms/ValidationMessage.ts:23:29 - error TS2339: Property 'classList' does not exist on type 'never'.

   23                     element.classList.add('is-invalid');
                               ~~~~~~~~~


   js/AppFramework/Forms/ValidationMessage.ts:25:29 - error TS2339: Property 'classList' does not exist on type 'never'.

   25                     element.classList.remove('is-invalid');
                               ~~~~~~~~~

Expected behavior:
No compilation error. In the if branch as shown in the compilation error, element was never re-assigned to 'never'. Also, the branch is not impossible to execute which would otherwise trigger this error.

Actual behavior:
Compilation error occurs:
TS2339: Property 'classList' does not exist on type 'never'

Playground Link:
Text too large to share, so created Gist instead with full repro:
https://gist.github.com/Sebazzz/aaa19b9793ba80e34acd5e900a3e0c81

If you put both the .d.ts file and .ts file in the playground, the offending lines are highlighted however. For fully repro I did include the tsconfig but it does not appear to be relevant.

@mhegazy
Copy link
Contributor

mhegazy commented Jan 31, 2018

This is caused by #15256, where the in expression is now used as a type guard. the condition 'setCustomValidity' in element narrows the type of element to never in the false branch, since HTMLInputElement is guaranteed to have a setCustomValidity.

@mhegazy mhegazy added the Breaking Change Would introduce errors in existing code label Jan 31, 2018
@Sebazzz
Copy link
Author

Sebazzz commented Feb 1, 2018

Right, but this check is for browser compatibility. The correct solution would be support that, or override this check locally?

@evil-shrike
Copy link

evil-shrike commented Feb 1, 2018

I also have TS starts failing on my code:

  isTouchDevice (): boolean {
    return !!('ontouchstart' in window) 	// works on most browsers
              || (window.navigator && window.navigator.msMaxTouchPoints > 0); // IE10
  }

with Error:(580, 36) TS2339: Property 'navigator' does not exist on type 'never'.
for window.navigator

Sebazzz added a commit to Sebazzz/financial-app that referenced this issue Feb 1, 2018
@NN---
Copy link

NN--- commented Feb 3, 2018

@evil-shrike What is really needed is lib.ie.d.ts.
In my project I already started to write it with few IE only functions.
I think it is a good idea to create this definition and add it to TS or DefinitelyTyped.

@NN---
Copy link

NN--- commented Feb 3, 2018

@evil-shrike I opened a new repo for lib.ie: https://github.com/NN---/lib.ie.d.ts/ .
I am going to use it in my project first and push to DefinitelyTyped once it has enough content.

@Sebazzz
Copy link
Author

Sebazzz commented Feb 4, 2018

That is not a viable solution. It would mean you would need a lib.d.ts for every browser combination.

@NN---
Copy link

NN--- commented Feb 4, 2018

@Sebazzz I don't see why it is bad. In any case you need type definitions to be type safe.
Otherwise just use any.

@sandersn
Copy link
Member

After some discussion in the typescript room, here are a couple of conclusions we reached.

1

The best solution is to use a workaround to avoid narrowing at all -- you don't really want narrowing behaviour here because (1) element doesn't have a union type (2) you already know about the compatibility issue, so you don't need an error warning you about it. I suggest

if ('setCustomValidity' in element as any)

2

This is technically a DOM bug, but not one that's feasible to fix. To be typesafe, the DOM types should be modelled as IE6HtmlInputElement | IE7HtmlInputElement ... | Chrome44HtmlInputElement ... Each present-day type would become a union of hundreds of nearly identical types. This is the worst case for compiler performance and would be confusing to use as well. And it's a breaking change for anybody with strictNullChecks on.

Alternatively, setCustomValidity could be made optional to reflect that it might or might not be present, but this too is a breaking change for those with strictNullChecks. This approach doesn't scale in the long-term, because all additions to the DOM would have to be optional. In 20 years we'd still be forced to check for the existence of properties that had been standard for decades.

@Sebazzz
Copy link
Author

Sebazzz commented Feb 15, 2018 via email

@mhegazy mhegazy closed this as completed Mar 9, 2018
@microsoft microsoft locked and limited conversation to collaborators Jul 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Breaking Change Would introduce errors in existing code
Projects
None yet
Development

No branches or pull requests

5 participants