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

Feature/UpgradeReactIsPackage #2300

Merged
merged 5 commits into from
Dec 9, 2019
Merged

Conversation

lh0x00
Copy link
Contributor

@lh0x00 lh0x00 commented Nov 30, 2019

I updated to the new version of react-is@16.12, it had some disruptions and I fixed it.

@lh0x00
Copy link
Contributor Author

lh0x00 commented Nov 30, 2019

image
why test passed on my mac? but in the Travis, it was failed!

@JeremyRH
Copy link

JeremyRH commented Dec 3, 2019

@lamhieu-vk it is a lint error here:
https://github.com/airbnb/enzyme/blob/430eb0eaa1f68e3787e208a650aac2f0cbd4fbfc/packages/enzyme-adapter-react-14/src/ReactFourteenAdapter.js#L117

enzyme: > eslint --ext js,jsx . "--quiet"
enzyme-adapter-react-14: /home/travis/build/airbnb/enzyme/packages/enzyme-adapter-react-14/src/ReactFourteenAdapter.js
enzyme-adapter-react-14:   117:22  error  Do not depend on the return value from ReactDOM.render  react/no-render-return-value
enzyme-adapter-react-14: ✖ 1 problem (1 error, 0 warnings)

I don't know why the lint task passed locally but you can find the suggested fix here:
https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/no-render-return-value.md

@lh0x00
Copy link
Contributor Author

lh0x00 commented Dec 4, 2019

I don't change anything in that files :<

@lh0x00
Copy link
Contributor Author

lh0x00 commented Dec 4, 2019

hi @ljharb , after eslint-plugin-react received an update at jsx-eslint/eslint-plugin-react@da2c7d2, in CI will throw an error about this rule. we can update logic for this flow after? In this branch, I just disabled in the above error code.

@JeremyRH
Copy link

JeremyRH commented Dec 4, 2019

@lamhieu-vk I'm not sure that logic needs to change as it uses an older version of ReactDOM that will always return a value from the render method. If anything, this logic needs to be refactored in the latest adapters:
https://github.com/airbnb/enzyme/blob/430eb0eaa1f68e3787e208a650aac2f0cbd4fbfc/packages/enzyme-adapter-react-16/src/ReactSixteenAdapter.js#L430

It seems this does not cause a lint error in CI yet so it's probably not a pressing issue now and can be handled later.

@lh0x00
Copy link
Contributor Author

lh0x00 commented Dec 8, 2019

It's ready to review!

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Thanks! I'll restructure the commits and get this in.

@@ -133,6 +133,7 @@ class ReactThirteenAdapter extends EnzymeAdapter {
};
const ReactWrapperComponent = createMountWrapper(el, { ...options, adapter });
const wrappedEl = React.createElement(ReactWrapperComponent, wrapperProps);
// eslint-disable-next-line react/no-render-return-value
Copy link
Member

@ljharb ljharb Dec 9, 2019

Choose a reason for hiding this comment

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

let's disable this in each package's .eslintrc (https://github.com/airbnb/enzyme/blob/master/packages/enzyme-adapter-react-13/.eslintrc) rather than inline, since it's disabling being OK depends on the react version.

however, i'm not seeing this as a warning or an error locally - why include it?

@ljharb ljharb merged commit 8b94a44 into enzymejs:master Dec 9, 2019
@bdwain
Copy link
Contributor

bdwain commented Dec 18, 2019

thanks for fixing this @lamhieu-vk and @ljharb. Are there any plans to do an enzyme release to get this out soon?

ljharb added a commit that referenced this pull request Dec 19, 2019
  - [fix] `isMemo` and `isLazy` are no longer directly useful for us as of `react-is` `v16.12` (#2300)
  - [deps] update `enzyme-shallow-equal`, `enzyme-adapter-utils`, `object-values`, `react-is`
  - [meta] add `funding` field
  - [dev deps] update `eslint`, `eslint-plugin-import`, `eslint-plugin-markdown`, `eslint-plugin-react`, `safe-publish-latest`
@ljharb
Copy link
Member

ljharb commented Dec 20, 2019

v3.11.0 has been released, as have relevant adapters.

@lh0x00
Copy link
Contributor Author

lh0x00 commented Dec 20, 2019

@ljharb thanks so much ^^

@bdwain
Copy link
Contributor

bdwain commented Dec 20, 2019

Awesome thanks

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

Successfully merging this pull request may close these issues.

None yet

5 participants