Fixed ColumnSpec.editor type

[jskupsik]

• Now properly types the "params" variable of the given editor function

Hoist P/R Checklist

Pull request authors: Review and check off the below. Items that do not apply can also be
checked off to indicate they have been considered. If unclear if a step is relevant, please leave
unchecked and note in comments.

• Caught up with develop branch as of last change.
• Added CHANGELOG entry, or determined not required.
• Reviewed for breaking changes, added breaking-change label + CHANGELOG if so.
• Updated doc comments / prop-types, or determined not required.
• Reviewed and tested on Mobile, or determined not required.
• Created Toolbox branch / PR, or determined not required.

If your change is still a WIP, please use the "Create draft pull request" option in the split
button below to indicate it is not ready yet for a final review.

Pull request reviewers: when merging this P/R, please consider using a squash commit to
collapse multiple intermediate commits into a single commit representing the overall feature
change. This helps keep the commit log clean and easy to scan across releases. PRs containing a
single commit should be rebased when possible.

[jskupsik]

This fixes an issue with typescript and this format:

editor: props => selectEditor({
  ...props,
  // There is absolutely no typescript type checking here
  inputProps: {
      options: ['A', 'B']
  }
}),

props was of type any, so the ...props essentially removed type checking. With this fix, props correctly contains the things necessary to satisfy EditorProps (other than the inputProps, which is supposed to be provided inline).

I do not understand why we were explicitly specifying FunctionComponent, when FunctionComponent<T extends ColumnEditorFnParams> satisfies the type ColumnEditorFn and no other functionality of FunctionComponent was used in the implementation. This simplification should have no loss of generality.

[amcclain]

@jskupsik should this still be a WIP draft, or is this ready for a review and (hopefully soon after) merge?

[jskupsik]

Ready to be merged.

[amcclain]

Thanks much Jakub. @TomTirapani and @ghsolomon - if either of you have a chance to review (and merge if looking good), would appreciate it. Nice to keep the incremental TS improvements flowing.

[jskupsik]

Note, ts will complain if you specify:
editor: props => selectEditor(props)
due to missing inputProps. I think this is fine because we support the syntax:
editor: selectEditor

Alternatively, we can make EditorProps.inputProps optional and allow both types to work.

Thoughts?

[TomTirapani]

Thanks for putting this PR together, a couple of thoughts:

a) Should we consider adding the FunctionComponent type back in (or FunctionComponent<T extends ColumnEditorFnParams>)? I understand its not strictly necessary, but one role of Typescript is to help with documentation and I think it might be clearer. Currently in this branch the comments talk about providing either a "Cell editor Component or a function to create one", and at first glance it seems a little confusing to just see the ColumnEditorFn type.

b) Yes, I think we should make inputProps optional to allow both syntaxes above to work. Beyond that it just seems more correct - they are optional if you want to use the defaults.

[ghsolomon]

I agree with @TomTirapani but also see a couple other Grid types that are essentially aliases for FunctionComponent:

• GroupRowRenderer
• ColumnHeaderNameFn

It seems like maybe all of these should use the FunctionComponent generic, and we could continue to export our aliases as to not introduce a “breaking change”?

[jskupsik]

I have a different way that has it working:

    editor?: FunctionComponent<ColumnEditorProps> | ColumnEditorFn;

with the trick this time being:

export type ColumnEditorFn = (props: ColumnEditorProps, nil?:any) => ReactElement;

I was wrong, the issue actually was that ColumnEditorFn was (a: ColumnEditorProps) => ReactElement while FunctionComponent had (a: ColumnEditorProps, b: any) => ReactElement.

Adding a dummy second argument to ColumnEditorFn, typed as ?:any but which will always be undefined, allows for IntelliJ to resolve that the first argument (in EITHER mode) will be ColumnEditorProps.

Thoughts on this version?

[amcclain]

Checking to see if we should catch up this PR and revisit? Looks like we've made some changes to the related types on develop, but not quite what we have here?