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

Don't suggest undefined | null properties #43117

Closed
5 tasks done
wojpawlik opened this issue Mar 6, 2021 · 8 comments
Closed
5 tasks done

Don't suggest undefined | null properties #43117

wojpawlik opened this issue Mar 6, 2021 · 8 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@wojpawlik
Copy link

Suggestion

πŸ” Search Terms

undefined null properties autocomplete autocompletion suggest

βœ… Viability Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, new syntax sugar for JS, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.

πŸ“ƒ Motivating Example

Union/Strict is very useful for undiscriminated unions. It adds ?: undefined / ?: never properties to all the variants, so

There's just one problem:

After narrowing [...], suggestions are bloated with multiple ?: undefined properties.

expected

actual

https://codesandbox.io/s/ts-toolbelt-forked-fomds

⭐ Suggestion

Hide undefined | null properties from suggestions.

undefined is more readable than actual.bar anyway.

@RA80533
Copy link

RA80533 commented Mar 6, 2021

The issue you're having is related specifically to the ts-toolbelt package. You might have better luck opening an issue in their repository for what you're dealing with.

@MartinJohns
Copy link
Contributor

@RA80533 I'm pretty sure the purpose of that library / the type is to explicitly provide the properties from the other union types with an undefined type, and OP is simply asking to not show properties of types when the property type is undefined (which IMO would be really weird behavior).

@RA80533
Copy link

RA80533 commented Mar 6, 2021

It appears so. In ts-toolbelt/Strict.ts, we can see that Union.Strict merges the unions together using the OptionalFlat type defined elsewhere in the library.

@wojpawlik
Copy link
Author

@MartinJohns understands. Moving the undefined | null properties to the bottom of the list and somehow graying them out would be fine too and perhaps less "weird", but it probably needs more code.

@RA80533 the issue is not specific to ts-toolbelt. I ended up implementing essentially the same type. Same problem, no fix.

@RA80533
Copy link

RA80533 commented Mar 6, 2021

@wojpawlik If I am understanding your problem correctly, I believe it is implementation-specific. That would mean that, just as it existed in ts-toolbelt, the problem would exist in your implementation if it was written the same way.

Take a look at the following snippet:

export type Result<TValue, TError = unknown> = Result.Success<TValue> | Result.Failure<TError>;

export namespace Result {
  
  export class Success<T> {
    public constructor (public readonly value: T) {}
    
    public isSuccess(): this is Result.Success<T> {
      return this.constructor.name === Result.Success.name;
    }
    
    public isFailure(): this is Result.Failure<T> & never {
      return false;
    }
  }
  
  export class Failure<T> {
    public constructor (public readonly error: T) {}
    
    public isSuccess(): this is Result.Success<T> & never {
      return false;
    }
    
    public isFailure(): this is Result.Failure<T> {
      return this.constructor.name === Result.Failure.name;
    }
  }
  
}
declare function f<T>(): Result<T>;

TypeScript will hint at the existence of isSuccess() and isFailure() on the return value of f(). Both type guards further elaborate the true shape of the value: a truthful isSuccess() adds value to the list of hinted properties while a isFailure() adds error to the list of hinted properties, etc.

Is this the kind of behavior you're looking for?

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Mar 8, 2021
@RyanCavanaugh
Copy link
Member

It's already possible to filter out undefined-inhabited properties in the type system, so if a utility type can do that if that's what should be happening.

I think it would be very confusing to do this in general; we can't assume what's intended by this sort of property.

@wojpawlik
Copy link
Author

I'm suggesting to just hide it from suggestions without affecting typechecking, not to disallow accessing it.

I don't think there's a way to narrow a value and end up with less properties than one started with.

I'm trying to accurately type an API with an undiscriminated union with 35 variants (no classes @RA80533). My proposal to add discriminators was rejected. And Union/Strict is the blessed way for EPC in non-discriminated unions, it should act like it!

@typescript-bot
Copy link
Collaborator

This issue has been marked 'Working as Intended' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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

5 participants