Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "Fix react useEffect dependencies in composites",
"packageName": "@internal/react-composites",
"email": "2684369+JamesBurnside@users.noreply.github.com",
"dependentChangeType": "patch"
}
2 changes: 1 addition & 1 deletion packages/communication-react/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@
"generate-doc": "api-documenter markdown -i temp -o docGen",
"prettier": "prettier --no-error-on-unmatched-pattern --write --config ../../.prettierrc --ignore-path=../../.prettierignore \"**/*.js\" \"**/*.jsx\" \"**/*.ts\" \"**/*.tsx\"",
"prettier:check": "prettier --no-error-on-unmatched-pattern --check --config ../../.prettierrc --ignore-path=../../.prettierignore \"**/*.js\" \"**/*.jsx\" \"**/*.ts\" \"**/*.tsx\"",
"lint": "eslint \"*/**/*.{ts,tsx}\"",
"lint": "eslint --max-warnings 0 \"*/**/*.{ts,tsx}\"",
"lint:fix": "npm run lint -- --fix",
"lint:quiet": "npm run lint -- --quiet",
"postpack": "copyfiles -E \"./*.tgz\" release",
Expand Down
2 changes: 1 addition & 1 deletion packages/react-composites/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
"generate-doc": "api-documenter markdown -i temp -o docGen",
"prettier": "prettier --no-error-on-unmatched-pattern --write --config ../../.prettierrc --ignore-path=../../.prettierignore \"**/*.js\" \"**/*.jsx\" \"**/*.ts\" \"**/*.tsx\"",
"prettier:check": "prettier --no-error-on-unmatched-pattern --check --config ../../.prettierrc --ignore-path=../../.prettierignore \"**/*.js\" \"**/*.jsx\" \"**/*.ts\" \"**/*.tsx\"",
"lint": "eslint \"*/**/*.{ts,tsx}\"",
"lint": "eslint --max-warnings 0 \"*/**/*.{ts,tsx}\"",
"lint:fix": "npm run lint -- --fix",
"lint:quiet": "npm run lint -- --quiet",
"_api-extractor:by-flavor": "if-env COMMUNICATION_REACT_FLAVOR=stable && api-extractor run -c api-extractor.stable.json --local || (if-env COMMUNICATION_REACT_FLAVOR=beta && api-extractor run --local)",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export type CallControlsProps = {
* @private
*/
export const CallControls = (props: CallControlsProps): JSX.Element => {
const options = typeof props.options === 'boolean' ? {} : props.options;
const options = useMemo(() => (typeof props.options === 'boolean' ? {} : props.options), [props.options]);
const customButtons = useMemo(
() => generateCustomButtons(onFetchCustomButtonPropsTrampoline(options), options?.displayType),
[options]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export const EndCall = (props: {
compactMode ? groupCallLeaveButtonCompressedStyle : groupCallLeaveButtonStyle,
props.styles ?? {}
),
[props.styles]
[compactMode, props.styles]
);
return (
<EndCallButton
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,15 +82,15 @@ const disableLobbyPageControls = (
};

const overlayProps = (strings: CallCompositeStrings, inLobby: boolean): LobbyOverlayProps =>
inLobby ? overlayPropsWaitingToBeAdmitted(strings, inLobby) : overlayPropsConnectingToCall(strings, inLobby);
inLobby ? overlayPropsWaitingToBeAdmitted(strings) : overlayPropsConnectingToCall(strings);

const overlayPropsConnectingToCall = (strings: CallCompositeStrings, inLobby: boolean): LobbyOverlayProps => ({
const overlayPropsConnectingToCall = (strings: CallCompositeStrings): LobbyOverlayProps => ({
title: strings.lobbyScreenConnectingToCallTitle,
moreDetails: strings.lobbyScreenConnectingToCallMoreDetails,
overlayIcon: <CallCompositeIcon iconName="LobbyScreenConnectingToCall" />
});

const overlayPropsWaitingToBeAdmitted = (strings: CallCompositeStrings, inLobby: boolean): LobbyOverlayProps => ({
const overlayPropsWaitingToBeAdmitted = (strings: CallCompositeStrings): LobbyOverlayProps => ({
title: strings.lobbyScreenWaitingToBeAdmittedTitle,
moreDetails: strings.lobbyScreenWaitingToBeAdmittedMoreDetails,
overlayIcon: <CallCompositeIcon iconName="LobbyScreenWaitingToBeAdmitted" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export const videoTileStyles = {
/**
* @private
*/
export const moreDetailsStyle = (palette, isVideoReady): IStyle => ({
export const moreDetailsStyle = (palette: IPalette, isVideoReady: boolean): IStyle => ({
fontSize: '1rem',
color: isVideoReady ? 'white' : palette.neutralPrimary,
textAlign: 'center'
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

import { FontWeights, IStyle } from '@fluentui/react';
import { FontWeights, IPalette, IStyle } from '@fluentui/react';

/**
* @private
Expand All @@ -22,7 +22,7 @@ export const titleContainerStyle: IStyle = {
/**
* @private
*/
export const titleStyle = (palette, isVideoReady): IStyle => ({
export const titleStyle = (palette: IPalette, isVideoReady: boolean): IStyle => ({
fontSize: '1.25rem',
fontWeight: FontWeights.semibold,
color: isVideoReady ? 'white' : palette.neutralPrimary,
Expand All @@ -32,7 +32,7 @@ export const titleStyle = (palette, isVideoReady): IStyle => ({
/**
* @private
*/
export const moreDetailsStyle = (palette, isVideoReady): IStyle => ({
export const moreDetailsStyle = (palette: IPalette, isVideoReady: boolean): IStyle => ({
fontSize: '1rem',
color: isVideoReady ? 'white' : palette.neutralPrimary,
textAlign: 'center'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -149,14 +149,14 @@ const CallWithChatScreen = (props: CallWithChatScreenProps): JSX.Element => {
const onMoreButtonClicked = useCallback(() => {
closePane();
setShowDrawer(true);
}, []);
}, [closePane]);
const closeDrawer = useCallback(() => {
setShowDrawer(false);
}, []);
const onMoreDrawerPeopleClicked = useCallback(() => {
setShowDrawer(false);
togglePeople();
}, []);
}, [togglePeople]);
const selectPeople = useCallback(() => {
setShowPeople(true);
setShowChat(false);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export const ChatButton = (props: ControlBarButtonProps): JSX.Element => {
},
props.styles ?? {}
),
[theme]
[props.styles, theme.palette.neutralLight]
);
return (
<ControlBarButton
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ export const ChatButtonWithUnreadMessagesBadge = (props: ChatButtonWithUnreadMes
{baseIcon}
</Stack>
);
}, [unreadChatMessagesCount, newMessageLabel]);
}, [unreadChatMessagesCount, newMessageLabel, baseIcon]);

useEffect(() => {
if (isChatPaneVisible) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ export const EmbeddedPeoplePane = (props: {
modalLayerHostId: string;
mobileView?: boolean;
}): JSX.Element => {
const { callAdapter, chatAdapter, inviteLink } = props;
const { callAdapter, chatAdapter, inviteLink, onFetchParticipantMenuItems } = props;
const participantListDefaultProps = usePropsFor(ParticipantList);

const callWithChatStrings = useCallWithChatCompositeStrings();
Expand All @@ -75,12 +75,12 @@ export const EmbeddedPeoplePane = (props: {
participant,
callWithChatStrings,
participantListDefaultProps.onRemoveParticipant,
participantListProps.myUserId
participantListDefaultProps.myUserId
);
if (props.onFetchParticipantMenuItems) {
contextualMenuItems = props.onFetchParticipantMenuItems(
if (onFetchParticipantMenuItems) {
contextualMenuItems = onFetchParticipantMenuItems(
participant.userId,
participantListProps.myUserId,
participantListDefaultProps.myUserId,
contextualMenuItems
);
}
Expand All @@ -90,7 +90,12 @@ export const EmbeddedPeoplePane = (props: {
setDrawerMenuItems(drawerMenuItems);
}
};
}, []);
}, [
callWithChatStrings,
participantListDefaultProps.onRemoveParticipant,
participantListDefaultProps.myUserId,
onFetchParticipantMenuItems
]);

const participantListProps: ParticipantListProps = useMemo(() => {
const onRemoveParticipant = async (participantId: string): Promise<void> =>
Expand All @@ -102,7 +107,7 @@ export const EmbeddedPeoplePane = (props: {
// We want the drawer menu items to appear when participants in ParticipantList are clicked
onParticipantClick: props.mobileView ? setDrawerMenuItemsForParticipant : undefined
};
}, [participantListDefaultProps, callAdapter, chatAdapter]);
}, [participantListDefaultProps, props.mobileView, setDrawerMenuItemsForParticipant, callAdapter, chatAdapter]);

const participantList = (
<ParticipantListWithHeading
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ export const PeopleButton = (props: ControlBarButtonProps): JSX.Element => {
},
props.styles ?? {}
),
[theme]
[props.styles, theme.palette.neutralLight]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: I tend to think of theme as an atomic unit. I'd not expect Contoso to modify parts of the theme so it's OK to add theme as the dependency (especially if the list of theme fields we reference starts growing)

);

return (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,16 +55,17 @@ export interface MoreDrawerProps extends MoreDrawerDevicesMenuProps {
export const MoreDrawer = (props: MoreDrawerProps): JSX.Element => {
const drawerMenuItems: DrawerMenuItemProps[] = [];

const onSelectSpeaker = useCallback(
const { speakers, onSelectSpeaker } = props;
const onSpeakerItemClick = useCallback(
(_ev, itemKey) => {
const selected = props.speakers?.find((speaker) => speaker.id === itemKey);
const selected = speakers?.find((speaker) => speaker.id === itemKey);
if (selected) {
// This is unsafe - we're only passing in part of the argument to the handler.
// But this is a known issue in our state.
props.onSelectSpeaker(selected as AudioDeviceInfo);
onSelectSpeaker(selected as AudioDeviceInfo);
}
},
[props.speakers, props.onSelectSpeaker]
[speakers, onSelectSpeaker]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why did you need to do this?

);

if (props.speakers && props.speakers.length > 0) {
Expand All @@ -80,23 +81,24 @@ export const MoreDrawer = (props: MoreDrawerProps): JSX.Element => {
: 'MoreDrawerSpeakers'
},
text: speaker.name,
onItemClick: onSelectSpeaker,
onItemClick: onSpeakerItemClick,
secondaryIconProps: isDeviceSelected(speaker, props.selectedSpeaker) ? { iconName: 'Accept' } : undefined
})),
secondaryText: props.selectedSpeaker?.name
});
}

const onSelectMicrophone = useCallback(
const { microphones, onSelectMicrophone } = props;
const onMicrophoneItemClick = useCallback(
(_ev, itemKey) => {
const selected = props.microphones?.find((mic) => mic.id === itemKey);
const selected = microphones?.find((mic) => mic.id === itemKey);
if (selected) {
// This is unsafe - we're only passing in part of the argument to the handler.
// But this is a known issue in our state.
props.onSelectMicrophone(selected as AudioDeviceInfo);
onSelectMicrophone(selected as AudioDeviceInfo);
}
},
[props.microphones, props.onSelectMicrophone]
[microphones, onSelectMicrophone]
);

if (props.microphones && props.microphones.length > 0) {
Expand All @@ -112,7 +114,7 @@ export const MoreDrawer = (props: MoreDrawerProps): JSX.Element => {
: 'MoreDrawerMicrophones'
},
text: mic.name,
onItemClick: onSelectMicrophone,
onItemClick: onMicrophoneItemClick,
secondaryIconProps: isDeviceSelected(mic, props.selectedMicrophone) ? { iconName: 'Accept' } : undefined
})),
secondaryText: props.selectedMicrophone?.name
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ export const ChatComposite = (props: ChatCompositeProps): JSX.Element => {
* @TODO Remove this function and pass the props directly when file-sharing is promoted to stable.
* @private
*/
// eslint-disable-next-line @typescript-eslint/explicit-function-return-type
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You can use FileSharingOptions return type. The type is defined unconditionally (the field ChatCompositeOptions.fileSharing is conditionally defined.

const fileSharingOptions = () => {
/* @conditional-compile-remove(file-sharing) */
return {
Expand Down
2 changes: 1 addition & 1 deletion tools/check-treeshaking/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
"test": "echo skip",
"test:stable": "echo skip",
"test:coverage": "echo skip",
"lint": "eslint \"**/*.{js,ts,tsx}\"",
"lint": "eslint --max-warnings 0 \"**/*.{js,ts,tsx}\"",
"lint:fix": "npm run lint -- --fix",
"lint:quiet": "npm run lint -- --quiet"
},
Expand Down