react-autosuggest: restrict input props

[OliverJAsh]

This brings InputProps inline with React's types for input. That is, the following errors in React:

<input anything={false} />

This also means that keyof InputProps<T> returns a union of literals instead of string. This is helpful because it means that keys will be validated in operations such as Pick<InputProps<T>, 'autoFocus'>.

---

Please fill in this template.

• Use a meaningful title for the pull request. Include the name of the package modified.
• Test the change in your own code. (Compile and run.)
• Add or edit tests to reflect the change. (Run with npm test.)
• Follow the advice from the readme.
• Avoid common mistakes.
• Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If changing an existing definition:

• Provide a URL to documentation or source code which provides context for the suggested changes: <>
• If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.
• If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }. If for reason the any rule need to be disabled, disable it for that line using // tslint:disable-next-line [ruleName] and not for whole package so that the need for disabling can be reviewed.

[typescript-bot]

@OliverJAsh Thank you for submitting this PR!

🔔 @nicolas-schmitt @pjo256 @robessog @tbayne @cdeutsch @rosskevin @ThomasdenH @ulrichb - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

[typescript-bot]

A definition owner has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped!

[typescript-bot]

👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings.

Let’s review the numbers, shall we?

Comparison details 📊

	master	#39186	diff
Batch compilation
Memory usage (MiB)	125.3	132.8	+5.9%
Type count	23994	23998	0%
Assignability cache size	53952	53953	0%
Language service
Samples taken	712	719	+1%
Identifiers in tests	712	719	+1%
getCompletionsAtPosition
Mean duration (ms)	694.0	697.9	+0.6%
Mean CV	7.2%	7.4%
Worst duration (ms)	959.8	907.0	-5.5%
Worst identifier	relatedTarget	theme
getQuickInfoAtPosition
Mean duration (ms)	646.6	647.4	+0.1%
Mean CV	8.2%	8.0%	-2.5%
Worst duration (ms)	844.3	865.2	+2.5%
Worst identifier	onBlur	className

It looks like nothing changed too much. I won’t post performance data again unless it gets worse.

[thenovelnomad]

This may be a case of your bug is my feature, but this broke our build process, and while I won't argue that it should not have been changed, it maybe should not have just been a patch version change.

[andrewbranch]

Hm, sorry to hear that @thenovelnomad. Unfortunately, the way @types versions work is that the major and minor versions track the JS library, and then the patch number increments every time there’s a change to the typings that doesn’t reflect a new major/minor version of the JS library. When typings are updated to be “more accurate,” that often means stricter, which could mean breaks for consumers. The only way to avoid shipping those breaks on a patch version is to wait for the JS library to release a new major or minor and ship the typings improvement after bumping its version to match the new JS version. That probably should have been considered here, but it’s not always feasible to do. 😕

[thenovelnomad]

Ahh, thanks for the details, @andrewbranch. That makes sense. I guess we should lock down our types packages to the patch version.

[dolezel]

This is definitely not correct as everything you pass as inputProps into the component you get as param of renderInputComponent

[dolezel]

https://github.com/mui-org/material-ui/blob/master/docs/src/pages/components/autocomplete/IntegrationAutosuggest.js
this throws type errors now...

[OliverJAsh]

@dolezel Please can you provide a reduced test case demonstrating the error you're seeing, in a new issue?

[dolezel]

I've posted link above. E.g. here https://github.com/mui-org/material-ui/blob/master/docs/src/pages/components/autocomplete/IntegrationAutosuggest.js#L199 it complains that 'classes' does not exist in type 'InputProps', but it should be possible to pass anything through and access it (e.g. https://github.com/mui-org/material-ui/blob/master/docs/src/pages/components/autocomplete/IntegrationAutosuggest.js#L50)

[OliverJAsh]

I see. In that case, InputProps should probably be a generic that is shared with renderInputComponent. Until someone is able to make that change, I would suggest using a cast on the properties which are causing this problem (e.g. classes):

inputProps={{
  // We want to pass these extra props through to `renderInputComponent`. A cast is needed because they are not included in the `InputProps` type.
  ...{ classes } as InputProps<T>,
  id: 'whatever',
  // other properties
}}

[ulrichb]

Ahhh renderInputComponent, I see.

In this case I would recommend to add the ref member not to InputProps but to the RenderInputComponent callback type (line 123).
Something like (inputProps: InputProps<TSuggestion> & { readonly ref: (instance: HTMLInputElement) => void }) => ....

This would avoid adding it to InputProps thus as input value for the AutoSuggest component (which would be misleading/wrong because it will get overwritten by AutoSuggest).

[cdeutsch]

Since renderInputComponent is a thing and you can render components from other libraries in it, I don't think [key: string]: any; should have been removed.

This broke the code I was using with Ant Design, which supports props like prefix and suffix

[lemieszek]

It also broke our implementation with data- prefixed properties.
It's valid in React so why should it be removed?
Maybe we should extend from a different interface to maintain backward compatibility as this change was "patch" but was breaking change for us.

[OliverJAsh]

InputProps should probably be a generic that is shared with renderInputComponent

This is the correct fix that doesn't remove type safety. Would someone like to send a PR? I'm happy to review it.

[lemieszek]

dataset is valid thing in HTML. Yet, we still can't define regex on properties names so we can't type safety date-name properties.
If we could do that (there is a proposal: microsoft/TypeScript#6579), it would be so easy to implement:

// DOM data attributes later used in HTMLOrSVGElement.dataset
interface DataAttributes {
    [key: /data-\w+/]: number | string | boolean;
}

yet as for now, we could do:

// DOM data attributes later used in HTMLOrSVGElement.dataset
interface DataAttributes {
    [key: string]: string;
}

But it will break whole type safety on everything 🙈