reflect-metadata icon indicating copy to clipboard operation
reflect-metadata copied to clipboard

Issue with IsConstructor when used with connect from react-redux

Open aletc1 opened this issue 5 years ago • 7 comments

Line 543 should not check IsConstructor.

exporter("deleteMetadata", deleteMetadata);
        function DecorateConstructor(decorators, target) {
            for (var i = decorators.length - 1; i >= 0; --i) {
                var decorator = decorators[i];
                var decorated = decorator(target);
                if (!IsUndefined(decorated) && !IsNull(decorated)) {
                    if (!IsConstructor(decorated))
                        throw new TypeError();
                    target = decorated;
                }
            }
            return target;
        }

image

As you can see on the image, decorated is a react class, hence typeof decorated is object and not function.

aletc1 avatar Apr 18 '19 18:04 aletc1

A React element is an object, but a React class component is definitely a function and a constructor. The issue here is that React.memo doesn't return a function - you need to decorate the React component before wrapping it in memo.

ljharb avatar Apr 18 '19 18:04 ljharb

Can you provide an example of the class being decorated? DecorateConstructor is expected to be called with a function as its target (the "constructor"), so the IsConstructor call looks to be correct.

rbuckton avatar Apr 18 '19 18:04 rbuckton

Yes, I will prepare some test. What I have found so far, is that react-redux v5 indeed returns a function... but react-redux v7 (using the same code) returns that object. It might not be an issue with reflect-metadata at all but with react-redux. Thanks for your prompt support.

aletc1 avatar Apr 18 '19 18:04 aletc1

@rbuckton I think you may close this issue. reflect-metadata works as expected (sorry to bother you since the issue is from react-redux). If someone stumbles with this issue (related to react-redux) the problem is:

  • Previous version of react-redux properly implements a connect class decorator (e.g react-redux 5.0.7) therefore, connect returns a wrapped function following class decorator specs. image
  • New version of react-redux (e.g react-redux 7.0.2) doesnt implements decorator logic and directly returns a react class (not a constructor function), and that is why IsConstructor fails in reflect-metadata. image

I ended up solving this by using the connectAdvanced function (not the connect one) and properly implementing the decorator.

Another workaround is to stick with version react-redux 5.0.7

aletc1 avatar Apr 18 '19 21:04 aletc1

It seems like there are two options that control the output of connectAdvanced that affect whether it can be used as a decorator:

  • options.pure - When true, wraps the created constructor in a call to React.memo. Default: true.
  • options.forwardRef - When true, wraps the previous result in a call to React.forwardRef. Default: false.

Its possible using ~@connect({ pure: false, ...otherOptions })~ might work for you. It depends on whether connect is truly intended to be used as a decorator, as it seems that may no longer be the case.

Edit: sorry, try this instead:

import { createConnect } from 'react-redux/lib/connect/connect';
const connect = createConnect({ pure: false });

@connect(...)
class Foo {}

rbuckton avatar Apr 18 '19 21:04 rbuckton

Awesome! Your tip works like a charm. Direct importing createConnect did not work for me, but if you pass { pure: false } as fourth param in connect, it works 👍

function decorator(args) {
  // ... configure mapStateToProps, etc based on some args or pass them directly
  return  connect(
     mapStateToProps,
     mapDispatchToProps,
     null,
     { pure: false })
)

@decorator(...)
class Foo {}

aletc1 avatar Apr 18 '19 22:04 aletc1

@rbuckton @aletc1

Setting pure: false does work, but at the cost of not having your connected components being pure, which means you lose performance. See: https://react-redux.js.org/7.1/api/connect#pure-boolean

I think the best solution is not using connect as decorator. Moreover if one uses TypeScript, connect as decorator throws compiler errors and you can't even find a proper fix because it's discouraged in all issues I found. So, I'd simply use hoc or hooks pattern instead, although refactoring to the first will be much faster.

With hoc pattern, you can use compose method from Redux, see: https://redux.js.org/api/compose

import { compose } from "redux"

export default compose(
  connect,
  withSubscribe,
  // other Hocs...
)(MyComponent)

adi518 avatar Jun 30 '20 13:06 adi518

Closing this as External. Class decorators should always return either undefined, or a replacement constructor. Returning a non-callable object is not supported in either TypeScript's --experimentalDecorators mode, nor in the Stage 3 ECMAScript Decorators proposal.

rbuckton avatar Dec 14 '23 20:12 rbuckton