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

[DataGridPro] Type the event publisher and listener #3022

Merged
merged 72 commits into from
Nov 27, 2021

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Oct 29, 2021

Preview: https://deploy-preview-3022--material-ui-x.netlify.app/components/data-grid/events/#catalog-of-events

The goal is to have a strict typing on our event publisher / listener arguments. This have several benefits:

  • hep us tracking errors in our code
  • have a single source of truth to link event name, params and event object
  • improve 3rd party developer DX when using subscribeEvent
  • allow to auto-generate documentation basic examples

Changes

  • Use the lookup to generate the description of the event name instead of hardcoding it
  • Use the lookup to define the prop callback type (props.onRowClick)
  • Type useGridApiEventHandler
  • Type apiRef.current.publishEvent
  • Type apiRef.current.subscribeEvent
  • Accept GridEvents.someEvent and "someEvent" everywhere

We still have the signature of the prop callbacks hardcoded in the comment because otherwise the documentation is not generated correctly from the prop-types.
We could autogen the comments of the TS file at some point but I don't want to over-engineer.

I improved the event table in the doc with a small example of subscription than shows the types and the params available.
This part can be moved to a follow up PR if you want.

image

Typing of the event params

  • Type as narrow as possible if the event comes from the DOM / the internal code
  • Accept MuiBaseEvent if the event potentially come from a apiRef method (ex: rowEditCommit is triggered via commitRowChange)

Bug fixes

I found a few bugs / behavior inconsistencies after typing the events

  • The cellNavigationKeyDown event can receive a GridRowParams as params argument when fired by handleRowEditStop in useGridEditRows. In this scenario it does not have a field and should pick the 1st column. It was working but was not safe since it called getColumnIndex with field=undefined.

Public API changes :

  • onCellValueChange removed because never called
  • cellDragStart and cellDragEnd events removed because never published

I don't think any of this count as breaking change, it's only type modification and bug fixes.

Changelog

  • 🎁 Add strict typing to the event publisher and listener [DataGridPro] Type the event publisher and listener #3022 (@flaviendelangle)

    apiRef.current.subscribeEvent / apiRef.current.publishEvent and useGridApiEventHandler are now fully typed and gives you the correct arguments based on the event you are listening to or emitting.

    const handleRowClick: GridEventListener<'rowClick'> = (
      params, // has type `GridRowParams`
      event, // has type `MuiEvent<React.MouseEvent<HTMLElement>>
      details, // has type `GridCallbackDetails
    ) => {
      /* ... */
    };
    
    // with string event name
    apiRef.current.subscribeEvent('rowClick', handleRowClick)
    useGridApiEventHandler(apiRef, 'rowClick', handleRowClick)
    
    // or with enum event name
    apiRef.current.subscribeEvent(GridEvents.rowClick, handleRowClick)
    useGridApiEventHandler(apiRef, GridEvents.rowClick, handleRowClick)

@flaviendelangle flaviendelangle self-assigned this Oct 29, 2021
@flaviendelangle flaviendelangle marked this pull request as draft October 29, 2021 09:17
@github-actions
Copy link

github-actions bot commented Nov 1, 2021

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 1, 2021
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Nov 1, 2021
@github-actions
Copy link

github-actions bot commented Nov 1, 2021

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 1, 2021
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 23, 2021
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 23, 2021
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 23, 2021

events.push({
name: event.escapedName!,
description: renderMarkdownInline(description),
params: escapeHTML(linkify(eventProperties.params, documentedInterfaces, 'html')),
event: escapeHTML(`MuiEvent<${eventProperties.event ?? '{}'}>`),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This created one problem:

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, I did the escape on the docs instead of doing it during the build

…ap.js

Co-authored-by: Matheus Wichman <matheushw@outlook.com>
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 23, 2021
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 23, 2021
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 24, 2021
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 24, 2021
const onEvent: GridEventListener<GridEvents.${event.name}> =
(
${args.join('\n ')}
) => {...}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<TableCell style={{ borderBottom: 'unset' }}>
<div
dangerouslySetInnerHTML={{
__html: escapeHTML(event.description),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not formatting correctly.

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

…ap.js

Co-authored-by: Matheus Wichman <matheushw@outlook.com>
@flaviendelangle flaviendelangle merged commit 6efc5f9 into mui:master Nov 27, 2021
@flaviendelangle flaviendelangle deleted the type-events branch November 27, 2021 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module! typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants