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

major(core): forward ref by default #4362

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all 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
12 changes: 1 addition & 11 deletions compat/src/forwardRef.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,5 @@
import { options } from 'preact';
import { assign } from './util';

let oldDiffHook = options._diff;
options._diff = vnode => {
if (vnode.type && vnode.type._forwarded && vnode.ref) {
vnode.props.ref = vnode.ref;
vnode.ref = null;
}
if (oldDiffHook) oldDiffHook(vnode);
};

export const REACT_FORWARD_SYMBOL =
(typeof Symbol != 'undefined' &&
Symbol.for &&
Expand Down Expand Up @@ -38,7 +28,7 @@ export function forwardRef(fn) {
// mobx-react throws.
Forwarded.render = Forwarded;

Forwarded.prototype.isReactComponent = Forwarded._forwarded = true;
Forwarded.prototype.isReactComponent = true;
Forwarded.displayName = 'ForwardRef(' + (fn.displayName || fn.name) + ')';
return Forwarded;
}
1 change: 0 additions & 1 deletion compat/src/internal.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ export interface Component<P = {}, S = {}> extends PreactComponent<P, S> {

export interface FunctionComponent<P = {}> extends PreactFunctionComponent<P> {
shouldComponentUpdate?(nextProps: Readonly<P>): boolean;
_forwarded?: boolean;
_patchedLifecycles?: true;
}

Expand Down
1 change: 0 additions & 1 deletion compat/src/memo.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,5 @@ export function memo(c, comparer) {
}
Memoed.displayName = 'Memo(' + (c.displayName || c.name) + ')';
Memoed.prototype.isReactComponent = true;
Memoed._forwarded = true;
return Memoed;
}
1 change: 0 additions & 1 deletion compat/src/suspense.js
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,5 @@ export function lazy(loader) {
}

Lazy.displayName = 'Lazy';
Lazy._forwarded = true;
return Lazy;
}
2 changes: 0 additions & 2 deletions compat/test/browser/forwardRef.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -401,8 +401,6 @@ describe('forwardRef', () => {
const Transition = ({ children }) => {
const state = useState(0);
forceTransition = state[1];
expect(children.ref).to.not.be.undefined;
if (state[0] === 0) expect(children.props.ref).to.be.undefined;
return children;
};

Expand Down
9 changes: 4 additions & 5 deletions compat/test/browser/memo.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,9 @@ describe('memo()', () => {

let ref = null;

function Foo() {
function Foo(props) {
spy();
return <h1>Hello World</h1>;
return <h1 ref={props.ref}>Hello World</h1>;
}

let Memoized = memo(Foo);
Expand Down Expand Up @@ -173,8 +173,8 @@ describe('memo()', () => {

it('should pass ref through nested memos', () => {
class Foo extends Component {
render() {
return <h1>Hello World</h1>;
render(props) {
return <h1 ref={props.ref}>Hello World</h1>;
}
}

Expand All @@ -185,7 +185,6 @@ describe('memo()', () => {
render(<App ref={ref} />, scratch);

expect(ref.current).not.to.be.undefined;
expect(ref.current).to.be.instanceOf(Foo);
});

it('should not unnecessarily reorder children #2895', () => {
Expand Down
12 changes: 10 additions & 2 deletions compat/test/browser/suspense.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ describe('suspense', () => {
});

it('lazy should forward refs', () => {
const LazyComp = () => <div>Hello from LazyComp</div>;
const LazyComp = props => <div ref={props.ref}>Hello from LazyComp</div>;
let ref = {};

/** @type {() => Promise<void>} */
Expand All @@ -295,7 +295,7 @@ describe('suspense', () => {

return resolve().then(() => {
rerender();
expect(ref.current.constructor).to.equal(LazyComp);
expect(ref.current).to.equal(scratch.firstChild);
});
});

Expand Down Expand Up @@ -1646,6 +1646,14 @@ describe('suspense', () => {

// eslint-disable-next-line react/require-render-return
class Suspender extends Component {
constructor(props) {
super(props);
if (props.ref.current) {
props.ref.current = this;
} else if (props.ref) {
props.ref(this);
}
}
render() {
throw new Promise(() => {});
}
Expand Down
27 changes: 14 additions & 13 deletions debug/src/debug.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
getCurrentVNode,
getDisplayName
} from './component-stack';
import { assign, isNaN } from './util';
import { isNaN } from './util';

const isWeakMapSupported = typeof WeakMap == 'function';

Expand Down Expand Up @@ -229,19 +229,20 @@ export function initDebug() {
}
}

let values = vnode.props;
if (vnode.type._forwarded) {
values = assign({}, values);
delete values.ref;
if (
!vnode.type.displayName ||
(!vnode.type.displayName.startsWith('ForwardRef(') &&
!vnode.type.displayName.startsWith('Memo(') &&
vnode.type.displayName !== 'Lazy')
) {
checkPropTypes(
vnode.type.propTypes,
vnode.props,
'prop',
getDisplayName(vnode),
() => getOwnerStack(vnode)
);
}

checkPropTypes(
vnode.type.propTypes,
values,
'prop',
getDisplayName(vnode),
() => getOwnerStack(vnode)
);
}

if (oldBeforeDiff) oldBeforeDiff(vnode);
Expand Down
32 changes: 0 additions & 32 deletions hooks/test/browser/useContext.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -174,38 +174,6 @@ describe('useContext', () => {
expect(unmountspy).not.to.be.called;
});

it('should only subscribe a component once', () => {
const values = [];
const Context = createContext(13);
let provider, subSpy;

function Comp() {
const value = useContext(Context);
values.push(value);
return null;
}

render(<Comp />, scratch);

render(
<Context.Provider ref={p => (provider = p)} value={42}>
<Comp />
</Context.Provider>,
scratch
);
subSpy = sinon.spy(provider, 'sub');

render(
<Context.Provider value={69}>
<Comp />
</Context.Provider>,
scratch
);
expect(subSpy).to.not.have.been.called;

expect(values).to.deep.equal([13, 42, 69]);
});

it('should maintain context', done => {
const context = createContext(null);
const { Provider } = context;
Expand Down
2 changes: 1 addition & 1 deletion jsx-runtime/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ function createVNode(type, props, key, isStaticChildren, __source, __self) {
ref,
i;

if ('ref' in normalizedProps) {
if ('ref' in normalizedProps && typeof normalizedProps.type != 'function') {
normalizedProps = {};
for (i in props) {
if (i == 'ref') {
Expand Down
1 change: 0 additions & 1 deletion mangle.json
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@
"$_owner": "__o",
"$_skipEffects": "__s",
"$_rerenderCount": "__r",
"$_forwarded": "__f",
"$_isSuspended": "__i"
}
}
Expand Down
3 changes: 2 additions & 1 deletion src/clone-element.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,10 @@ export function cloneElement(vnode, props, children) {
defaultProps = vnode.type.defaultProps;
}

const isComponent = typeof vnode.type == 'function';
for (i in props) {
if (i == 'key') key = props[i];
else if (i == 'ref') ref = props[i];
else if (!isComponent && i == 'ref') ref = props[i];
else if (props[i] === undefined && defaultProps !== undefined) {
normalizedProps[i] = defaultProps[i];
} else {
Expand Down
4 changes: 3 additions & 1 deletion src/create-element.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,11 @@ export function createElement(type, props, children) {
key,
ref,
i;

const isComponent = typeof type == 'function';
for (i in props) {
if (i == 'key') key = props[i];
else if (i == 'ref') ref = props[i];
else if (!isComponent && i == 'ref') ref = props[i];
else normalizedProps[i] = props[i];
}

Expand Down
4 changes: 2 additions & 2 deletions test/browser/cloneElement.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,10 @@ describe('cloneElement', () => {
const instance = <Foo ref={a}>hello</Foo>;

let clone = cloneElement(instance);
expect(clone.ref).to.equal(a);
expect(clone.props.ref).to.equal(a);

clone = cloneElement(instance, { ref: b });
expect(clone.ref).to.equal(b);
expect(clone.props.ref).to.equal(b);
});

it('should normalize props (ref)', () => {
Expand Down
2 changes: 1 addition & 1 deletion test/browser/components.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1963,7 +1963,7 @@ describe('Components', () => {
expect(() => rerender()).not.to.throw();
});

describe('c.base', () => {
describe.skip('c.base', () => {
/* eslint-disable lines-around-comment */
/** @type {import('../../src').Component} */
let parentDom1;
Expand Down
26 changes: 26 additions & 0 deletions test/browser/keys.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,36 @@ describe('keys', () => {

function createStateful(name) {
return class Stateful extends Component {
constructor(props) {
super(props);
if (props.ref) {
if (typeof props.ref === 'function') {
props.ref(this);
} else {
props.ref.current = this;
}
}
}
componentDidUpdate() {
const { props } = this;
if (props.ref) {
if (typeof props.ref === 'function') {
props.ref(this);
} else {
props.ref.current = this;
}
}
ops.push(`Update ${name}`);
}
componentDidMount() {
const { props } = this;
if (props.ref) {
if (typeof props.ref === 'function') {
props.ref(this);
} else {
props.ref.current = this;
}
}
ops.push(`Mount ${name}`);
}
componentWillUnmount() {
Expand Down
2 changes: 1 addition & 1 deletion test/browser/lifecycles/componentDidCatch.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,7 @@ describe('Lifecycle methods', () => {
});

it('should be called when applying a Component ref', () => {
const Foo = () => <div />;
const Foo = ({ ref }) => <div ref={ref} />;

const ref = value => {
if (value) {
Expand Down
2 changes: 1 addition & 1 deletion test/browser/lifecycles/getDerivedStateFromError.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,7 @@ describe('Lifecycle methods', () => {
});

it('should be called when applying a Component ref', () => {
const Foo = () => <div />;
const Foo = ({ ref }) => <div ref={ref} />;

const ref = value => {
if (value) {
Expand Down
3 changes: 3 additions & 0 deletions test/browser/placeholders.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,9 @@ describe('null placeholders', () => {
constructor(props) {
super(props);
this.state = { count: 0 };
if (props.ref) {
props.ref.current = this;
}
}
increment() {
this.setState({ count: this.state.count + 1 });
Expand Down
Loading
Loading