-
Notifications
You must be signed in to change notification settings - Fork 26.8k
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
Use react-is to check component validity #5095
Conversation
Looks like the tests failed, at least:
You can run them manually using:
|
aa0decc
to
ceff43f
Compare
I wonder if we can make this check development only, and only load react-is in that case to save bundle size |
That sounds like a good idea. Does Next.js have an existing pattern to do that? One way is to add a NODE_ENV check before the if (process.env.NODE_ENV === 'development' && !isValidElementType(Component)) {
throw new Error(`The default export is not a React Component in page: "${pathname}"`)
} I'm trying this out, adding this to {
include: /react-is/,
sideEffects: false,
}, But I don't have too much experience using webpack's tree shaking, so not sure if I'm missing something. |
I guess something like this should work: if (process.env.NODE_ENV === 'development') {
const {isValidElementType} = require('react-is')
if(!isValidElementType(Component)) {
throw new Error(`The default export is not a React Component in page: "${pathname}"`)
}
} In general, I always wrap the |
The test previously used the string value "not-a-page" to verify that Next.js renders the error "The default export is not a React Component". This no longer works because "not-a-page" is technically an allowable element now, which renders an HTML element called <not-a-page>. Fixed by replacing the value with an object: { not: "a-page" }
ceff43f
to
51d3b5b
Compare
Trying it out now, and it appears that react-is is still included in the production bundle even with the suggested outer I've pushed my changes to this branch, and you can verify with:
Open http://localhost:3000 and look at the commons JS asset (mine is http://localhost:3000/_next/static/chunks/commons.792d7d9de2a7d11ba82f.js). You'll find the minified source of react-is in here. |
I wonder if it’s shipped with React? |
I’ll have a look tomorrow 👍🏻 |
I suspect that the reason why webpack/uglify's dead code elimination is not working is that Next.js's compiled sources use CJS modules instead of ES modules. e.g. in var _reactIs = _interopRequireDefault(require("react-is")); Not 100% sure though; I'll have more time to verify this weekend. |
Uglify's dead code elimination works by removing blocks that have |
Fixed in 5ddd91c I had to add IgnorePlugin for that specific file. Apparently webpack still bundles the require otherwise 🙏 |
Awesome, thanks 🙏 |
Looks like the tests are failing though. |
Looks like the
The test suite doesn't enter the Should I update the test to expect React's error instead? Or maybe change the test suite so specific tests are executed using |
Hello 😄This PR seems great 👍 Do you think we could consider having react-is in the production bundle if/when the tree-shakeable version is merged and released facebook/react#13321 Or is it better to keep react-is outside of the production bundle and we should rather change the tests ? |
I'm going to close this PR as it has merge conflicts and the test was breaking, but I'll add good first issue / help wanted labels to the original issue and post that you already implement most here so that someone can take a look 💯 |
…#5857) Resolves #4055 Credit: #5095 I didn't use the ignore webpack plugin from the original PR and tested bundle size with #5339 - seems to be safe on that front. Was able to get tests to pass locally, unsure of what goes wrong in CI 🤷♂️ **Questions** 1) The initial PR didn't include changes to `next-server/lib/router` in `getRouteInfo()`. Should the same changes be made within? 2) Should we add a test for rendering a component created via `forwardRef()`? `component-with-forwardedRef`: ```javascript export default React.forwardRef((props, ref) => <span {...props} forwardedRef={ref}>This is a component with a forwarded ref</span>); ``` some test: ```javascript test('renders from forwardRef', async () => { const $ = await get$('/component-with-forwardedRef') const span = $('span') expect(span.text()).toMatch(/This is a component with a forwarded ref/) }) ```
Fixes #4055
To try this out, check out my branch, then:
Then edit
examples/hello-world/pages/index.js
:Visit http://localhost:3000 and the example should render without error.