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

Address XS code hygiene tasks #4980

Closed
wants to merge 1 commit into from
Closed

Conversation

OEvgeny
Copy link
Collaborator

@OEvgeny OEvgeny commented Dec 20, 2023

part of #4968

Changelog Entry

Description

Address the first 3 suggestions from #4968

Design

Specific Changes

List of changes:

  • XS: Do not use React FC/VFC types, use Props instead
  • XS: Mark all props as read-only, also any arrays and objects under it
  • XS: Prefer import { type ABC } over import type { ABC }

As we remove FC/VFC I thought it'd be nice to add ReactNode return type basically to:

  • Serve as an additional hint that the function follows React Component signature
  • Limit usage of unsupported by React runtime return types

-

  • I have added tests and executed them locally
  • I have updated CHANGELOG.md
  • I have updated documentation

Review Checklist

This section is for contributors to review your work.

  • Accessibility reviewed (tab order, content readability, alt text, color contrast)
  • Browser and platform compatibilities reviewed
  • CSS styles reviewed (minimal rules, no z-index)
  • Documents reviewed (docs, samples, live demo)
  • Internationalization reviewed (strings, unit formatting)
  • package.json and package-lock.json reviewed
  • Security reviewed (no data URIs, check for nonce leak)
  • Tests reviewed (coverage, legitimacy)

Address the first 3 suggestions from microsoft#4968

As we remove `FC/VFC` I thought it'd be nice to add `ReactNode` return type basically to:
- Serve as an additional hint that the function follows React Component signature
- Limit usage of unsupported by React runtime return types

type ActivityAcknowledgementComposerProps = PropsWithChildren<{}>;
type ActivityAcknowledgementComposerProps = Readonly<PropsWithChildren<{}>>;
Copy link
Contributor

@compulim compulim Dec 20, 2023

Choose a reason for hiding this comment

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

This is my bad and lack of understanding about TypeScript couple of years ago. Could you help change this line to:

Suggested change
type ActivityAcknowledgementComposerProps = Readonly<PropsWithChildren<{}>>;
type ActivityAcknowledgementComposerProps = Readonly<{ children?: ReactNode | undefined }>;

In TypeScript, {} means any. And it means I can put whatever inside the props. Recently learn about type-fest package for TypeScript helpers. One of the helpers can define an empty object. 😉


type ActivityIdToKeyMap = Map<string, string>;
type ActivityToKeyMap = Map<WebChatActivity, string>;
type ClientActivityIdToKeyMap = Map<string, string>;
type KeyToActivityMap = Map<string, WebChatActivity>;

type ActivityKeyerComposerProps = Readonly<PropsWithChildren<{}>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
type ActivityKeyerComposerProps = Readonly<PropsWithChildren<{}>>;
type ActivityKeyerComposerProps = Readonly<{ children?: ReactNode | undefined }>;


// Magic numbers for `expiryByActivityKey`.
const EXPIRY_SEND_FAILED = -Infinity;
const EXPIRY_SENT = Infinity;

const ActivitySendStatusComposer: FC<PropsWithChildren<{}>> = ({ children }) => {
type ActivitySendStatusComposerProps = Readonly<PropsWithChildren<{}>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
type ActivitySendStatusComposerProps = Readonly<PropsWithChildren<{}>>;
type ActivitySendStatusComposerProps = Readonly<{ children?: ReactNode | undefined }>;


type ActivityTreeComposerProps = PropsWithChildren<{}>;
type ActivityTreeComposerProps = Readonly<PropsWithChildren<{}>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
type ActivityTreeComposerProps = Readonly<PropsWithChildren<{}>>;
type ActivityTreeComposerProps = Readonly<{ children?: ReactNode | undefined }>;

@OEvgeny
Copy link
Collaborator Author

OEvgeny commented Dec 20, 2023

Thanks William, good catch indead. I'll recreate the PR under the webchat repo for the CI PR pipeline to work

@OEvgeny OEvgeny closed this Dec 20, 2023
@OEvgeny OEvgeny deleted the chore/xs-tasks branch January 5, 2024 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants