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

feat!: React 17 support #538

Merged
merged 5 commits into from
Jan 18, 2022
Merged

feat!: React 17 support #538

merged 5 commits into from
Jan 18, 2022

Conversation

nissy-dev
Copy link
Contributor

Fixed #450

I added "plugin:react/jsx-runtime" for React 17 based on the following statement.

If you are using the new JSX transform from React 17, extend react/jsx-runtime in your eslint config (add "plugin:react/jsx-runtime" to "extends") to disable the relevant rules.

https://github.com/yannickcr/eslint-plugin-react#configuration

Copy link
Contributor

@koba04 koba04 left a comment

Choose a reason for hiding this comment

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

👍 LGTM!!!

Copy link
Contributor

@koba04 koba04 left a comment

Choose a reason for hiding this comment

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

Could you fix failing tests?

@nissy-dev
Copy link
Contributor Author

nissy-dev commented Jan 14, 2022

@koba04
I think there are two ways of passing the tests.

  1. just removing import React from "react" in tests
  2. enable "react/jsx-uses-react" rules

Which is better? I seem that 1. way leads to breaking backward compatibility.
Some projects should remove all unused React import statements.

Copy link
Member

@sakit0 sakit0 left a comment

Choose a reason for hiding this comment

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

LGTM🎉

@koba04
Copy link
Contributor

koba04 commented Jan 14, 2022

@nissy-dev I prefer the option 1.
This is definitely a breaking change, but React v17 was released over a year, so I think it makes sense to drop React v16 support. We can use the codemod script the React team provides for the migration.

https://reactjs.org/blog/2020/09/22/introducing-the-new-jsx-transform.html#removing-unused-react-imports

But we should provide an announcement for the breaking change that we can still use the classic JSX syntax (React.createElement) by enabling the react/jsx-uses-react rule.
So let's add a note for the classic JSX syntax in README.


⚠️ Class JSX Syntax

@cybozu/eslint-config is intented to be used with the New JSX Transform. If you want to use the Classic JSX Transform (React.createElement), please enable the react/jsx-uses-react rule on your own.

rules: {
  "react/jsx-uses-react": "error"
}

README.md Outdated
@@ -123,3 +123,13 @@ module.exports = {
```

We also provide `@cybozu/eslint-config/presets/kintone-customize-es5-prettier` to use it with `prettier`.

## ⚠️ Class JSX Syntax
Copy link
Contributor

Choose a reason for hiding this comment

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

@nissy-dev
Thank you! I have an additional request 🙏
I want to add the ## React Support section and move this to the upper of the ## For kintone customize developers section.

Suggested change
## ⚠️ Class JSX Syntax
## React Support
### ⚠️ Classic JSX Syntax

Could you apply the change and move the section to the upper of the ## For kintone customize developers section?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@koba04
I fixed!

@koba04 koba04 changed the title React 17 support feat!: React 17 support Jan 18, 2022
Copy link
Contributor

@koba04 koba04 left a comment

Choose a reason for hiding this comment

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

👏 👏 👏 Great works!!!

@koba04 koba04 merged commit dffb83d into cybozu:master Jan 18, 2022
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.

Support for react v17
3 participants