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

Fix React Component detection #1604

Merged
merged 2 commits into from Feb 1, 2018

Conversation

Projects
None yet
2 participants
@edorivai
Copy link
Contributor

edorivai commented Jan 31, 2018

@leoasis

This comment has been minimized.

Copy link
Collaborator

leoasis commented Jan 31, 2018

@edorivai mind adding a test for this? It would be super useful so that we don't cause a regression accidentally.

Also, add an entry in the changelog that mentions this fix.

Also make sure to update this branch with what's latest in master

@edorivai

This comment has been minimized.

Copy link
Contributor Author

edorivai commented Feb 1, 2018

@leoasis 2 questions:

First, where in the changelog should I add the entry? Under 2.1.0-beta.0, or rather somewhere else?

Second, I'm having a bit of trouble with writing the test, I tried just copying over the basic classes test, and changing the render to be on the instance, instead of on the prototype:

      it('basic classes with render on instance', () => {
        let elementCount = 0;
        class MyComponent extends React.Component<any, any> {
          render = () => {
            return <div>{_.times(this.props.n, i => <span key={i} />)}</div>;
          }
        }
        walkTree(<MyComponent n={5} />, {}, element => {
          elementCount += 1;
        });
        expect(elementCount).toEqual(7);
      });

However, Typescript is too smart, and points out the "mistake":

TS2424: Class 'Component<any, any>' defines instance member function 'render', but extended class 'MyComponent' defines it as instance member property.

I tried just rendering the <Link/> element from react-scroll, which does the job. But I can imagine you'd prefer not to add react-scroll as a dev dependency.

Do you have any idea how to neatly test this?

@leoasis

This comment has been minimized.

Copy link
Collaborator

leoasis commented Feb 1, 2018

First, where in the changelog should I add the entry? Under 2.1.0-beta.0, or rather somewhere else?

Under vNext

However, Typescript is too smart, and points out the "mistake":

In this case I think it's fair to bypass Typescript since this is a use case for plain JS users:

class MyComponent extends (React.Component as any) {
  render = () => {
    return <div>{_.times(this.props.n, i => <span key={i} />)}</div>;
  }
}
@leoasis

This comment has been minimized.

Copy link
Collaborator

leoasis commented Feb 1, 2018

Also remember to bring the latest changes from master into your branch (either merge master into this branch or rebase this branch on top of master)

@edorivai edorivai force-pushed the edorivai:patch-1 branch from f1b945e to cbd93b0 Feb 1, 2018

@edorivai

This comment has been minimized.

Copy link
Contributor Author

edorivai commented Feb 1, 2018

@leoasis Thanks for the help, let me know if I still need to change anything 😅

@leoasis

This comment has been minimized.

Copy link
Collaborator

leoasis commented Feb 1, 2018

@edorivai awesome work, thanks!

@leoasis leoasis merged commit 4e7a87c into apollographql:master Feb 1, 2018

4 checks passed

CLA Author has signed the Meteor CLA.
Details
bundlesize ./lib/react-apollo.browser.umd.js: 5.98KB < maxSize 6KB (gzip)(7B larger than master, careful!)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 96.085%
Details
@edorivai

This comment has been minimized.

Copy link
Contributor Author

edorivai commented Feb 1, 2018

🎉

@leoasis final question, is there a plan for when a new release will be cut? We've currently disabled SSR on a couple pages until this patch lands. But if it will take a couple days before a release, we might have to look at alternative temporary solutions.

Thanks again 😄

@leoasis

This comment has been minimized.

Copy link
Collaborator

leoasis commented Feb 1, 2018

The idea would be to release in the next couple days, so yeah it should be soon.

Regarding a workaround, why not try defining the render method in the prototype for now? You can define it as an empty function, it won't matter since the other one is defined in the instance and will be looked at first:

import { Link } from 'react-scroll';

Link.prototype.render = () => {};

I think that should be enough.

Edit: Hmmm now that I look at the source there it is wrapped in an HoC so not sure that will work since you can't access the underlying component.

@edorivai

This comment has been minimized.

Copy link
Contributor Author

edorivai commented Feb 1, 2018

@leoasis Heh, that would have been such a nice hack. Next couple days should be fine.

@leoasis

This comment has been minimized.

Copy link
Collaborator

leoasis commented Feb 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment