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

feat!: add StrictMode support #5687

Merged
merged 3 commits into from
Mar 8, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/AccordionCollapse.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import classNames from 'classnames';
import * as React from 'react';
import { useContext } from 'react';
import PropTypes from 'prop-types';
import { Transition } from 'react-transition-group';
import { useBootstrapPrefix } from './ThemeProvider';
import Collapse, { CollapseProps } from './Collapse';
import AccordionContext from './AccordionContext';
Expand Down Expand Up @@ -29,7 +30,7 @@ const propTypes = {
const AccordionCollapse: BsPrefixRefForwardingComponent<
'div',
AccordionCollapseProps
> = React.forwardRef<typeof Collapse, AccordionCollapseProps>(
> = React.forwardRef<Transition<any>, AccordionCollapseProps>(
({ bsPrefix, className, children, eventKey, ...props }, ref) => {
const { activeEventKey } = useContext(AccordionContext);
bsPrefix = useBootstrapPrefix(bsPrefix, 'accordion-collapse');
Expand Down
13 changes: 9 additions & 4 deletions src/Carousel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import useCommittedRef from '@restart/hooks/useCommittedRef';
import useTimeout from '@restart/hooks/useTimeout';
import classNames from 'classnames';
import transitionEnd from 'dom-helpers/transitionEnd';
import Transition from 'react-transition-group/Transition';
import { TransitionStatus } from 'react-transition-group/Transition';
import PropTypes from 'prop-types';
import * as React from 'react';
import {
Expand All @@ -23,6 +23,7 @@ import SafeAnchor from './SafeAnchor';
import { useBootstrapPrefix } from './ThemeProvider';
import triggerBrowserReflow from './triggerBrowserReflow';
import { BsPrefixProps, BsPrefixRefForwardingComponent } from './helpers';
import TransitionWrapper from './TransitionWrapper';

export type CarouselVariant = 'dark';

Expand Down Expand Up @@ -539,14 +540,18 @@ const Carousel: BsPrefixRefForwardingComponent<
const isActive = index === renderedActiveIndex;

return slide ? (
<Transition
<TransitionWrapper
in={isActive}
onEnter={isActive ? handleEnter : undefined}
onEntered={isActive ? handleEntered : undefined}
addEndListener={transitionEnd}
>
{(status) =>
{(
status: TransitionStatus,
innerProps: Record<string, unknown>,
) =>
React.cloneElement(child, {
...innerProps,
className: classNames(
child.props.className,
isActive && status !== 'entered' && orderClassName,
Expand All @@ -556,7 +561,7 @@ const Carousel: BsPrefixRefForwardingComponent<
),
})
}
</Transition>
</TransitionWrapper>
) : (
React.cloneElement(child, {
className: classNames(
Expand Down
21 changes: 11 additions & 10 deletions src/Collapse.tsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import classNames from 'classnames';
import css from 'dom-helpers/css';
import PropTypes from 'prop-types';
import * as React from 'react';
import { useMemo } from 'react';
import React, { useMemo } from 'react';
import Transition, {
TransitionStatus,
ENTERED,
ENTERING,
EXITED,
Expand All @@ -13,6 +13,7 @@ import transitionEndListener from './transitionEndListener';
import { TransitionCallbacks } from './helpers';
import createChainedFunction from './createChainedFunction';
import triggerBrowserReflow from './triggerBrowserReflow';
import TransitionWrapper from './TransitionWrapper';

type Dimension = 'height' | 'width';

Expand Down Expand Up @@ -150,7 +151,7 @@ const defaultProps = {
getDimensionValue: getDefaultDimensionValue,
};

const Collapse = React.forwardRef(
const Collapse = React.forwardRef<Transition<any>, CollapseProps>(
(
{
onEnter,
Expand All @@ -163,7 +164,7 @@ const Collapse = React.forwardRef(
dimension = 'height',
getDimensionValue = getDefaultDimensionValue,
...props
}: CollapseProps,
},
ref,
) => {
/* Compute dimension */
Expand Down Expand Up @@ -219,8 +220,7 @@ const Collapse = React.forwardRef(
);

return (
<Transition
// @ts-ignore
<TransitionWrapper
ref={ref}
addEndListener={transitionEndListener}
{...props}
Expand All @@ -230,19 +230,20 @@ const Collapse = React.forwardRef(
onEntered={handleEntered}
onExit={handleExit}
onExiting={handleExiting}
childRef={(children as any).ref}
Copy link
Member

Choose a reason for hiding this comment

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

this will always be falsey, children, if it's an element would have ref on children.props not on the element itself

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm children.ref is actually defined when I was testing this. I've tried children.props.ref but I get a warning:

Warning: div: ref is not a prop. Trying to access it will result in undefined being returned. If you need to access the same value within the child component, you should pass it as a different prop.

Copy link
Member Author

Choose a reason for hiding this comment

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

tbh, children.ref did seem kinda hacky (esp. with the casting). I'm not 100% sure on what the correct approach is :P

Copy link
Member

Choose a reason for hiding this comment

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

o hmm i must have it backwards, been doing this for too long. I'm suprised the types don't correctly cover this tho

Copy link
Member Author

Choose a reason for hiding this comment

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

I found this on the react github project that seems to confirm the children.ref usage:

facebook/react#8873 (comment)

>
{(state, innerProps) =>
React.cloneElement(children as any, {
{(state: TransitionStatus, innerProps: Record<string, unknown>) =>
React.cloneElement(children, {
...innerProps,
className: classNames(
className,
(children as any).props.className,
children.props.className,
collapseStyles[state],
computedDimension === 'width' && 'width',
),
})
}
</Transition>
</TransitionWrapper>
);
},
);
Expand Down
11 changes: 7 additions & 4 deletions src/Fade.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,14 @@ import PropTypes from 'prop-types';
import * as React from 'react';
import { useCallback } from 'react';
import Transition, {
TransitionStatus,
ENTERED,
ENTERING,
} from 'react-transition-group/Transition';
import transitionEndListener from './transitionEndListener';
import { TransitionCallbacks } from './helpers';
import triggerBrowserReflow from './triggerBrowserReflow';
import TransitionWrapper from './TransitionWrapper';

export interface FadeProps extends TransitionCallbacks {
className?: string;
Expand Down Expand Up @@ -93,19 +95,20 @@ const Fade = React.forwardRef<Transition<any>, FadeProps>(
const handleEnter = useCallback(
(node) => {
triggerBrowserReflow(node);
if (props.onEnter) props.onEnter(node);
props.onEnter?.(node);
},
[props],
);

return (
<Transition
<TransitionWrapper
ref={ref}
addEndListener={transitionEndListener}
{...props}
onEnter={handleEnter}
childRef={(children as any).ref}
Copy link
Member

Choose a reason for hiding this comment

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

ditto

>
{(status, innerProps) =>
{(status: TransitionStatus, innerProps: Record<string, unknown>) =>
React.cloneElement(children, {
...innerProps,
className: classNames(
Expand All @@ -116,7 +119,7 @@ const Fade = React.forwardRef<Transition<any>, FadeProps>(
),
})
}
</Transition>
</TransitionWrapper>
);
},
);
Expand Down
34 changes: 15 additions & 19 deletions src/OverlayTrigger.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import useTimeout from '@restart/hooks/useTimeout';
import safeFindDOMNode from 'react-overlays/safeFindDOMNode';
import warning from 'warning';
import { useUncontrolledProp } from 'uncontrollable';
import useMergedRefs from '@restart/hooks/useMergedRefs';
import Overlay, { OverlayChildren, OverlayProps } from './Overlay';

export type OverlayTriggerType = 'hover' | 'click' | 'focus';
Expand Down Expand Up @@ -37,12 +38,6 @@ export interface OverlayTriggerProps
onHide?: never;
}

class RefHolder extends React.Component {
render() {
return this.props.children;
}
}

function normalizeDelay(delay?: OverlayDelay) {
return delay && typeof delay === 'object'
? delay
Expand Down Expand Up @@ -188,6 +183,10 @@ function OverlayTrigger({
...props
}: OverlayTriggerProps) {
const triggerNodeRef = useRef(null);
const mergedRef = useMergedRefs<unknown>(
triggerNodeRef,
(children as any).ref,
Copy link
Member

Choose a reason for hiding this comment

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

ditto

);
const timeout = useTimeout();
const hoverStateRef = useRef<string>('');

Expand All @@ -200,10 +199,9 @@ function OverlayTrigger({
? React.Children.only(children).props
: ({} as any);

const getTarget = useCallback(
() => safeFindDOMNode(triggerNodeRef.current),
[],
);
const attachRef = (r: React.ComponentClass | Element | null | undefined) => {
mergedRef(safeFindDOMNode(r));
};

const handleShow = useCallback(() => {
timeout.clear();
Expand Down Expand Up @@ -272,7 +270,9 @@ function OverlayTrigger({
);

const triggers: string[] = trigger == null ? [] : [].concat(trigger as any);
const triggerProps: any = {};
const triggerProps: any = {
ref: attachRef,
};

if (triggers.indexOf('click') !== -1) {
triggerProps.onClick = handleClick;
Expand All @@ -294,21 +294,17 @@ function OverlayTrigger({

return (
<>
{typeof children === 'function' ? (
children({ ...triggerProps, ref: triggerNodeRef })
) : (
<RefHolder ref={triggerNodeRef}>
{cloneElement(children as any, triggerProps)}
</RefHolder>
)}
{typeof children === 'function'
? children(triggerProps)
: cloneElement(children, triggerProps)}
<Overlay
{...props}
show={show}
onHide={handleHide}
flip={flip}
placement={placement}
popperConfig={popperConfig}
target={getTarget as any}
target={triggerNodeRef.current}
>
{overlay}
</Overlay>
Expand Down
93 changes: 93 additions & 0 deletions src/TransitionWrapper.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
import React, { useCallback, useRef } from 'react';
import Transition, {
TransitionProps,
TransitionStatus,
} from 'react-transition-group/Transition';
import safeFindDOMNode from 'react-overlays/safeFindDOMNode';
import useMergedRefs from '@restart/hooks/useMergedRefs';

export type TransitionWrapperProps = TransitionProps & {
childRef?: React.Ref<unknown>;
children:
| React.ReactElement
| ((
status: TransitionStatus,
props: Record<string, unknown>,
) => React.ReactNode);
};

// Normalizes Transition callbacks when nodeRef is used.
const TransitionWrapper = React.forwardRef<
Copy link
Member

Choose a reason for hiding this comment

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

I can never remember what RTG is doing now...do we need this wrapper? If it's in v5 can we just break the API and require node ref compatible components?

Copy link
Member Author

Choose a reason for hiding this comment

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

RTG has this quirk where the node is excluded in the callback signature when using nodeRef.

ie. (node, isAppearing) => ... becomes (isAppearing) => ...

This wrapper abstracts all that so callers don't have to worry about it (like they can just use it like it was without using nodeRef). I kinda figured this would be a cleaner approach.

Copy link
Member

Choose a reason for hiding this comment

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

ah, that seems like a bad API choice i probably ok'd oops

Transition<any>,
TransitionWrapperProps
>(
(
{
onEnter,
onEntering,
onEntered,
onExit,
onExiting,
onExited,
addEndListener,
children,
childRef,
...props
},
ref,
) => {
const nodeRef = useRef<HTMLElement>(null);
const mergedRef = useMergedRefs(nodeRef, childRef);

const attachRef = (
r: React.ComponentClass | Element | null | undefined,
) => {
mergedRef(safeFindDOMNode(r));
};

const normalize = (
callback?: (node: HTMLElement, param: unknown) => void,
) => (param: unknown) => {
if (callback && nodeRef.current) {
callback(nodeRef.current, param);
}
};

const handleEnter = useCallback(normalize(onEnter), [onEnter]);
const handleEntering = useCallback(normalize(onEntering), [onEntering]);
const handleEntered = useCallback(normalize(onEntered), [onEntered]);
const handleExit = useCallback(normalize(onExit), [onExit]);
const handleExiting = useCallback(normalize(onExiting), [onExiting]);
const handleExited = useCallback(normalize(onExited), [onExited]);
const handleAddEndListener = useCallback(normalize(addEndListener), [
addEndListener,
]);

return (
<Transition
ref={ref}
{...props}
onEnter={handleEnter}
onEntered={handleEntered}
onEntering={handleEntering}
onExit={handleExit}
onExited={handleExited}
onExiting={handleExiting}
addEndListener={handleAddEndListener}
nodeRef={nodeRef}
>
{typeof children === 'function'
? (status: TransitionStatus, innerProps: Record<string, unknown>) =>
children(status, {
...innerProps,
ref: attachRef,
})
: React.cloneElement(children as React.ReactElement, {
ref: attachRef,
})}
</Transition>
);
},
);

export default TransitionWrapper;
14 changes: 14 additions & 0 deletions test/CarouselSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,20 @@ describe('<Carousel>', () => {
<Carousel.Item key={3}>Item 3 content</Carousel.Item>,
];

it('should not throw an error with StrictMode', () => {
const ref = React.createRef();

mount(
<React.StrictMode>
<Carousel ref={ref} interval={null}>
{items}
</Carousel>
</React.StrictMode>,
);

ref.current.next();
});

it('should show the first item by default and render all', () => {
const wrapper = mount(<Carousel>{items}</Carousel>);

Expand Down