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

Compiled .d.ts output for Omit is verbose and semantically inconsistent #34793

Closed
craigkovatch opened this issue Oct 29, 2019 · 18 comments
Closed
Assignees
Labels
Fix Available A PR has been opened for this issue Needs Investigation This issue needs a team member to investigate its status. Rescheduled This issue was previously scheduled to an earlier milestone

Comments

@craigkovatch
Copy link

craigkovatch commented Oct 29, 2019

TypeScript Version: 3.7.1-rc (but not appreciably different in 3.5.1)

Search Terms: Omit, Pick, verbose, long

Code

// HtmlAttrs.ts
export type InputAttrs = React.InputHTMLAttributes<HTMLInputElement>;

// TextFieldWidget.tsx
import { InputAttrs } from '../../utils/HtmlAttrs';
export interface TextFieldProps {
  onChange?: (e: React.ChangeEvent<HTMLInputElement> & { isComposing: boolean }) => void;
}
export const TextFieldWidget = React.forwardRef<HTMLInputElement, TextFieldProps & Omit<InputAttrs, 'onChange'>>((props, ref) => {};

// Output in TextFieldWidget.d.ts
export declare const TextFieldWidget: React.ForwardRefExoticComponent<TextFieldProps & Pick<InputAttrs, "children" | "dir" | "form" | "slot" | "style" | "title" | "hidden" | "pattern" | "disabled" | "defaultChecked" | "defaultValue" | "suppressContentEditableWarning" | "suppressHydrationWarning" | "accessKey" | "className" | "contentEditable" | "contextMenu" | "draggable" | "id" | "lang" | "placeholder" | "spellCheck" | "tabIndex" | "inputMode" | "is" | "radioGroup" | "role" | "about" | "datatype" | "inlist" | "prefix" | "property" | "resource" | "typeof" | "vocab" | "autoCapitalize" | "autoCorrect" | "autoSave" | "color" | "itemProp" | "itemScope" | "itemType" | "itemID" | "itemRef" | "results" | "security" | "unselectable" | "aria-activedescendant" | "aria-atomic" | "aria-autocomplete" | "aria-busy" | "aria-checked" | "aria-colcount" | "aria-colindex" | "aria-colspan" | "aria-controls" | "aria-current" | "aria-describedby" | "aria-details" | "aria-disabled" | "aria-dropeffect" | "aria-errormessage" | "aria-expanded" | "aria-flowto" | "aria-grabbed" | "aria-haspopup" | "aria-hidden" | "aria-invalid" | "aria-keyshortcuts" | "aria-label" | "aria-labelledby" | "aria-level" | "aria-live" | "aria-modal" | "aria-multiline" | "aria-multiselectable" | "aria-orientation" | "aria-owns" | "aria-placeholder" | "aria-posinset" | "aria-pressed" | "aria-readonly" | "aria-relevant" | "aria-required" | "aria-roledescription" | "aria-rowcount" | "aria-rowindex" | "aria-rowspan" | "aria-selected" | "aria-setsize" | "aria-sort" | "aria-valuemax" | "aria-valuemin" | "aria-valuenow" | "aria-valuetext" | "dangerouslySetInnerHTML" | "onCopy" | "onCopyCapture" | "onCut" | "onCutCapture" | "onPaste" | "onPasteCapture" | "onCompositionEnd" | "onCompositionEndCapture" | "onCompositionStart" | "onCompositionStartCapture" | "onCompositionUpdate" | "onCompositionUpdateCapture" | "onFocus" | "onFocusCapture" | "onBlur" | "onBlurCapture" | "onChangeCapture" | "onBeforeInput" | "onBeforeInputCapture" | "onInput" | "onInputCapture" | "onReset" | "onResetCapture" | "onSubmit" | "onSubmitCapture" | "onInvalid" | "onInvalidCapture" | "onLoad" | "onLoadCapture" | "onError" | "onErrorCapture" | "onKeyDown" | "onKeyDownCapture" | "onKeyPress" | "onKeyPressCapture" | "onKeyUp" | "onKeyUpCapture" | "onAbort" | "onAbortCapture" | "onCanPlay" | "onCanPlayCapture" | "onCanPlayThrough" | "onCanPlayThroughCapture" | "onDurationChange" | "onDurationChangeCapture" | "onEmptied" | "onEmptiedCapture" | "onEncrypted" | "onEncryptedCapture" | "onEnded" | "onEndedCapture" | "onLoadedData" | "onLoadedDataCapture" | "onLoadedMetadata" | "onLoadedMetadataCapture" | "onLoadStart" | "onLoadStartCapture" | "onPause" | "onPauseCapture" | "onPlay" | "onPlayCapture" | "onPlaying" | "onPlayingCapture" | "onProgress" | "onProgressCapture" | "onRateChange" | "onRateChangeCapture" | "onSeeked" | "onSeekedCapture" | "onSeeking" | "onSeekingCapture" | "onStalled" | "onStalledCapture" | "onSuspend" | "onSuspendCapture" | "onTimeUpdate" | "onTimeUpdateCapture" | "onVolumeChange" | "onVolumeChangeCapture" | "onWaiting" | "onWaitingCapture" | "onAuxClick" | "onAuxClickCapture" | "onClick" | "onClickCapture" | "onContextMenu" | "onContextMenuCapture" | "onDoubleClick" | "onDoubleClickCapture" | "onDrag" | "onDragCapture" | "onDragEnd" | "onDragEndCapture" | "onDragEnter" | "onDragEnterCapture" | "onDragExit" | "onDragExitCapture" | "onDragLeave" | "onDragLeaveCapture" | "onDragOver" | "onDragOverCapture" | "onDragStart" | "onDragStartCapture" | "onDrop" | "onDropCapture" | "onMouseDown" | "onMouseDownCapture" | "onMouseEnter" | "onMouseLeave" | "onMouseMove" | "onMouseMoveCapture" | "onMouseOut" | "onMouseOutCapture" | "onMouseOver" | "onMouseOverCapture" | "onMouseUp" | "onMouseUpCapture" | "onSelect" | "onSelectCapture" | "onTouchCancel" | "onTouchCancelCapture" | "onTouchEnd" | "onTouchEndCapture" | "onTouchMove" | "onTouchMoveCapture" | "onTouchStart" | "onTouchStartCapture" | "onPointerDown" | "onPointerDownCapture" | "onPointerMove" | "onPointerMoveCapture" | "onPointerUp" | "onPointerUpCapture" | "onPointerCancel" | "onPointerCancelCapture" | "onPointerEnter" | "onPointerEnterCapture" | "onPointerLeave" | "onPointerLeaveCapture" | "onPointerOver" | "onPointerOverCapture" | "onPointerOut" | "onPointerOutCapture" | "onGotPointerCapture" | "onGotPointerCaptureCapture" | "onLostPointerCapture" | "onLostPointerCaptureCapture" | "onScroll" | "onScrollCapture" | "onWheel" | "onWheelCapture" | "onAnimationStart" | "onAnimationStartCapture" | "onAnimationEnd" | "onAnimationEndCapture" | "onAnimationIteration" | "onAnimationIterationCapture" | "onTransitionEnd" | "onTransitionEndCapture" | "size" | "multiple" | "list" | "step" | "autoFocus" | "type" | "height" | "formAction" | "formEncType" | "formMethod" | "formNoValidate" | "formTarget" | "name" | "value" | "width" | "alt" | "crossOrigin" | "src" | "checked" | "maxLength" | "readOnly" | "accept" | "autoComplete" | "capture" | "max" | "min" | "minLength" | "required"> & React.RefAttributes<HTMLInputElement>>;

Expected behavior:
I expect the .d.ts definition to mimic the structure of the original definition, i.e. to use Omit rather than Pick. In particular, this changes the self-documenting semantic of using Omit. The original definition is intended to make it obvious that the signature of the onChange event is not the standard definition from React, but an augmented type.

BTW -- there are more TextFieldProps in the real code. The point of defining them in a separate interface that doesn't extend the React props, and then combining the types in the component definition, is to make it easy to document, browse, and create objects containing the component-scope props, without them getting lost in the noise of 260+ native HTML props.

Actual behavior:
Instead, the Omit definition is replaced by the compile-time equivalent as a Pick statement. This defeats the self-documenting semantic that is intended by using Omit. It is very confusing to library consumers -- "why is there such a gigantic Pick type with 200+ entries???" And it actually changes the intended semantic, as the meaning of the Omit-based type is intended to be evaluated by the typescript compiler when the user of the library compiles, not when the library itself is compiled. e.g. if a new version of @types/react includes a new event type, I should not have to recompile/redistribute my library to support it.

I don't-know-I-don't-know WAY MORE about compilers than I know, so I am certainly open to education on this, but both of the issues raised here do seem like (subtle) bugs to me.

@AnyhowStep
Copy link
Contributor

AnyhowStep commented Oct 29, 2019

Somewhat related to #34556

If there was a way to control TS to alias, or not alias a type, one could just say,


alias type Omit<T, K> = Pick<T, Exclude<keyof T, K>>

Then, output will always use Omit<T, K>.


Or,

nonAlias type Omit<T, K> = Pick<T, Exclude<keyof T, K>>

Then, the output will always use Pick<T, Exclude<keyof T, K>>


Right now, whether a type alias gets "expanded" or not is a bit unintuitive.


Often times, I have to use the Identity<> trick. Otherwise, the output is 100s of lines long, when the "expanded" type is only 5 lines long or something.

@craigkovatch
Copy link
Author

@AnyhowStep thanks for your comment. I’m wondering from it — do you know of any existing workarounds to influence this unwanted behavior, or is your proposal necessary first?

@AnyhowStep
Copy link
Contributor

AnyhowStep commented Oct 29, 2019

Just write your own Omit type,

type Omit2<T, K extends keyof T> = {
    [P in Exclude<keyof T, K>]: T[P];
};

import * as React from "react";

// HtmlAttrs.ts
export type InputAttrs = React.InputHTMLAttributes<HTMLInputElement>;

// TextFieldWidget.tsx
export interface TextFieldProps {
  onChange?: (e: React.ChangeEvent<HTMLInputElement> & { isComposing: boolean }) => void;
}

/*
const TextFieldWidget: React.ForwardRefExoticComponent<
  TextFieldProps & 
  Omit2<InputAttrs, "onChange"> &
  React.RefAttributes<HTMLInputElement>
>
*/
export const TextFieldWidget =
  React.forwardRef<
  HTMLInputElement, 
  TextFieldProps & Omit2<InputAttrs, 'onChange'>
>((props, ref) => { };

Playground


It's a pretty difficult problem, in the general case.

Usually, the emit is not considered part of the behaviour of these type alises/operators.
Because it shouldn't matter what the emit is, as long as it the emitted types follow the expected assignability rules, have the right properties, etc. (Occasionally, there are bugs and the emitted types have a different behaviour from the original types. Whoops!)

However, when you add humans into the mix, suddenly, the emit kind of matters (readability).

Because sometimes, a 2 line emit is more readable than a 100+ line emit.
Other times, the 100+ line emit is more readable than the 2 line emit (it's possible, trust me).

You can have two types that have the exact same behaviour, except for having different emit.
And you cannot say, with certainty, which type is better. (For example, Omit<> vs Omit2<>)

The human using the types has to decide if type A or type B is better for a particular use case.
All this trouble, because these types have different emit, despite having the exact same behaviour.


I actually unit test the .d.ts output of my library now, because readability for the downstream consumer is kind of important to me =x

@craigkovatch
Copy link
Author

@AnyhowStep I’m actually using my own Omit definition as many of the modules depending on this module don’t have a new enough TSC to assume the built-in type. But the emit is just as verbose, whether I use the TS-provided def or my own. Any other ideas? :(

@AnyhowStep
Copy link
Contributor

AnyhowStep commented Oct 29, 2019

This output isn't verbose, though

Omit2<InputAttrs, "onChange">

What does your definition look like?

@craigkovatch
Copy link
Author

craigkovatch commented Oct 29, 2019

@AnyhowStep that's how I got here -- both the built-in Omit and mine give the same verbose output. I'm using the same definition that was "omitted" (lol) in TS 2.8 as "trivial":

export type Omit<T, K> = Pick<T, Exclude<keyof T, K>>;

@craigkovatch
Copy link
Author

I guess my expectation is that the compiler resolves (sorry if wrong term) the Omit type later when it's used, rather than resolving it in the .d.ts emit.

@AnyhowStep
Copy link
Contributor

Did you try my Omit2 implementation?

@fatcerberus
Copy link

It seems likely that this has the same root cause as #34777 -- specifically: #34777 (comment)

@craigkovatch
Copy link
Author

@RyanCavanaugh this got marked as 3.7.2 milestone but definitely did not go in 3.7.2. Just want to make sure that doesn't make it fall into some abyss...

@ExE-Boss
Copy link
Contributor

I think we should start with special‑casing Omit to always be preserved in IntelliSense and .d.ts emit.

This is because #35889 won’t work due to reasons outlined in #35889 (comment) by @dragomirtitian:

I am pretty sure this option was considered when Omit was added. It was rejected because it does not preserve optionality and readonly-ness. Pick is homomorphic since it maps over K where K extends keyof T and thus preserves modifiers.

Since your type maps over Exclude<keyof T, K> it is not homomorphic. Only types that map over keyof T where T is a type parameter or that map over K where K is type parameter with K extends keyof T are homomorphic.

type Omit2<T, K extends keyof any> = {
    [P in Exclude<keyof T, K>]: T[P];
};

type U = {
    a: number,
    readonly b?: number
} 

type R = Omit2<U, 'a'> // has b: number | undefined;
type R2 = Omit<U, 'a'> // has  readonly b?: number | undefined;

Playground Link

@AnyhowStep
Copy link
Contributor

Or we can just have some way of telling the compiler to not expand a type alias,
#35654

@ypresto
Copy link
Contributor

ypresto commented Jan 14, 2020

IMO Expanding type alias, especially for assignment of a variable type or a function return type, seems to be a current design limitation of checker/transformer/syntax.

  • Type references used in the current file written as-is to d.ts file.
  • Resolving type references visible in the current file scope might be relatively easy (but not implemented yet? or closed Print unions resulting from keyof operations with keyof if possible #20838 ?).
  • But to rid of all expansion from the root, we need to add type imports which type can be not-exported.
  • You also cannot get typeof generic function type. Passing type argument someFunction<T> to result of typeof someFunction without actual call of function is not possible.

Explicit alias/non-alias flag on the type and interface declaration does not solve the not-exported type problem.

@weswigham
Copy link
Member

weswigham commented Mar 25, 2020

@RyanCavanaugh The easy fix here for us is just to change the lib implementation of Omit to soemthing like

type Omit<T, K extends keyof any, Keys extends keyof T = Exclude<keyof T, K>> = {
    [P in Keys]: T[P];
};

rather than reusing Pick. Do we want to do this, and do we want to do this for 3.9? I'll open a quick PR with the change in the meantime.

@ExE-Boss
Copy link
Contributor

@weswigham See #35889 (comment) for why that didn’t work.

@weswigham
Copy link
Member

@ExE-Boss I edit sniped you with a corrected declaration that does work.

@MichaelBelousov
Copy link

MichaelBelousov commented May 21, 2020

FWIW I wanted to point out that I hit this issue and the proposed workaround of Omit2 does not work until typescript 3.9.

It looks to me like index signature types over a type using Omit always produce an exhaustive Pick complement which breaks a composed type in a private package I wrote where the input to Omit comes from a peer dependency, and therefore the generated key union type can be wrong. The outputted Pick is generated from my installed devDependencies, but should be resolved on the consumer's side with their installed peer dependencies. The proposed workaround not working means my package can only support typescript 3.9, but I might be able to move the generic type order to work around it.

Here is a minimal viable reproduction, with commits in the history confirming it works in 3.9 but not 3.8.

EDIT: looks like inverting the composition order does not help in 3.8

@RyanCavanaugh
Copy link
Member

This seems to work as expected in 5.0 -- the Omit is preserved in the output:

export declare const TextFieldWidget: import("react").ForwardRefExoticComponent<TextFieldProps & Omit<InputAttrs, "onChange"> & import("react").RefAttributes<HTMLInputElement>>;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fix Available A PR has been opened for this issue Needs Investigation This issue needs a team member to investigate its status. Rescheduled This issue was previously scheduled to an earlier milestone
Projects
None yet