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

[index.js] Preact-friendly class component check #31

Closed
wants to merge 4 commits into from

Conversation

petermikitsh
Copy link

Taking the same approach as acdlite/recompose@1c9cf7b, this PR augments the class checking logic to support Preact.

In my particular use case, I am using react-final-form-arrays (which depends on this project, though it is inlined in the distribution) (see line 59).

When using Preact, I get this exception:

[start:server] webpack-internal:///./node_modules/react-final-form-arrays/dist/react-final-form-arrays.es.js:68
[start:server]     throw new Error('Can only polyfill class components');
[start:server]     ^
[start:server] 
[start:server] Error: Can only polyfill class components
[start:server]     at polyfill (webpack-internal:///./node_modules/react-final-form-arrays/dist/react-final-form-arrays.es.js:68:11)
[start:server]     at eval (webpack-internal:///./node_modules/react-final-form-arrays/dist/react-final-form-arrays.es.js:512:1)

This PR fixes that thrown exception.

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

prototype && prototype.isReactComponent is the standard way (within the React codebase) that we check for React components. I think preact-compat supports this too by adding that property to its components. So I'm not sure why this change would be needed?

test.js Outdated
...BaseClass.getDerivedStateFromProps(nextProps, prevState),
bar: 'bar',
};
return Object.assign(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change? I think Node 8.3+ supports the spread operator, and our devEngines block specifies 8.5+ or 9+

Copy link
Author

Choose a reason for hiding this comment

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

@bvaughn Reverted this. When I first began work on this PR, I was using Node 8.0 on my machine. Switching to 8.5 fixed the issue.

@@ -35,5 +35,8 @@
},
"devEngines": {
"node": "8.5 || 9.x"
},
"jest": {
"testURL": "http://localhost"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change?

Copy link
Author

Choose a reason for hiding this comment

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

@bvaughn: I was getting this runtime error (running the test suite without this change) =>

> jest test.js

 FAIL  ./test.js
  ● Test suite failed to run

    SecurityError: localStorage is not available for opaque origins
      
      at Window.get localStorage [as localStorage] (node_modules/jsdom/lib/jsdom/browser/Window.js:257:15)
          at Array.forEach (<anonymous>)

Test Suites: 1 failed, 1 total
Tests:       0 total
Snapshots:   0 total
Time:        0.824s, estimated 4s

I've actually seen this error when using Jest in other projects. The solution I've come across (fixing the exception) is discussed in jestjs/jest#6766 (comment).

@petermikitsh
Copy link
Author

I'm not using preact-compat. I'm using preact (core). They don't define isReactComponent (see https://github.com/developit/preact/blob/master/src/preact.js).

In preact core, isReactComponent always evaluates to falsey and the error unnecessarily trips the exception. I agree that using isReactComponent is the conventional approach, but checking for a render function is functionally equivalent (unit test suites pass), with the added bonus of supporting preact. The latter is the primary reason for this PR.

@bvaughn
Copy link
Contributor

bvaughn commented Oct 1, 2018

I see.

I think there's value in keeping this logic consistent in all places (here and React core). It's been around for a long time and changing it might have unforeseen effects (false positives or negatives). For example, this is a valid React component but would fail the proposed check:

class Example extends React.Component {
  render = () => <div />; 
}

I also think preact-compat is a reasonable requirement in order for a Preact app to be able to use a package like react-lifecycles-compat.

Hope you understand. Thanks for taking the time to submit a PR and talk it through though!

@petermikitsh
Copy link
Author

Some interesting discussion (that I was not aware of prior to submitting to this PR):

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants