Skip to content

Conversation

@MCanhisares
Copy link
Contributor

@MCanhisares MCanhisares commented Apr 8, 2024

Summary:

Implements eslint flat config file support

Changelog:

[GENERAL] [ADDED] - Adding support for Flat Config files

Test Plan:

Run lint on a reproducer with the eslint config changes
Reproducer example: https://github.com/MCanhisares/eslint-flat-upgrade

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 8, 2024
@MCanhisares MCanhisares changed the title Updating eslint packages to flat file Updating eslint packages to flat config file Apr 8, 2024
@analysis-bot
Copy link

analysis-bot commented Apr 8, 2024

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 19,558,636 +0
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 22,928,758 -4
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: 3f17c8b
Branch: main

@MCanhisares MCanhisares marked this pull request as ready for review April 8, 2024 21:02
@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Apr 8, 2024
@jafar-jabr
Copy link

@MCanhisares Is there any chance that this will be merged soon ?

@MCanhisares
Copy link
Contributor Author

MCanhisares commented May 11, 2024

@cortinico what do we have to do to get this ready for merge?

Edit: To add more context, the JS tasks are failing because the eslint server is using the latest version of this package which should be ready for the new config format. Should we fix these issues in this PR? Or open another PR to handle the changes to the main proj?

@Jonnboy91
Copy link

Any update on this?

@robhogan
Copy link
Contributor

robhogan commented Jun 12, 2024

LGTM on the face of it, importing to test inside the FB monorepo.

We should bump the template devDependency to ^8.23.0 as well though - I'll add that.

@facebook-github-bot
Copy link
Contributor

@robhogan has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Copy link
Contributor

@robhogan robhogan left a comment

Choose a reason for hiding this comment

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

Hi @MCanhisares - thanks for this and sorry it's been in review for so long. Having had a closer look, this seems to break support for legacy config - this is actually the error CI is correctly reporting, and also came up importing to the internal monorepo - it requires users to update their projects to use flat config.

I think that's too much upgrade friction, and also presumably unintentional from the PR description - we should be able to support flat config without breaking legacy just yet, as eg jsx-eslint/eslint-plugin-react#3429 did.

@MCanhisares MCanhisares force-pushed the feat/eslint-flat-config branch from c16e55c to fa9fccd Compare August 9, 2024 01:24

| Eslint version | `.eslintrc` support | `eslint.config.js` support |
| :------------------- | :-----------------: | :------------------------: |
| `>= 9.0.0` |||
Copy link

Choose a reason for hiding this comment

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

green check in eslint.config.js column here?

@MCanhisares MCanhisares force-pushed the feat/eslint-flat-config branch from 10afaf3 to e2badfd Compare August 15, 2024 03:43
@MCanhisares
Copy link
Contributor Author

Hi @MCanhisares - thanks for this and sorry it's been in review for so long. Having had a closer look, this seems to break support for legacy config - this is actually the error CI is correctly reporting, and also came up importing to the internal monorepo - it requires users to update their projects to use flat config.

I think that's too much upgrade friction, and also presumably unintentional from the PR description - we should be able to support flat config without breaking legacy just yet, as eg jsx-eslint/eslint-plugin-react#3429 did.

Hi @robhogan, sorry this took a while to get back to, but I made the changes to support both legacy and new flat config formats

In order for this to work though, we need to change our approach for the configs, and export one that supports legacy .eslintrc projects and one for projects that use the new format (confirmed by one of the core eslint maintainers ).

Another problem arises from this, where eslint reads the package name to create an alias to the config itself, so we have two alternatives:

  1. Move away from the current package structure, and distribute the configs on the react-native-eslint-plugin package (seems like the better option, and is what the react, angular, solid and many others do)
  2. Create a new package eslint-config-react-native-flat and publish it so other projects can use the new config format.

The current code is uses approach 1, but I can easily migrate to 2 if you think it makes more sense

Let me know what you think, and also the tests might need to be updated to correctly read the new eslint configs. Where should I make those changes?

@cortinico cortinico requested a review from robhogan August 15, 2024 10:00
@theo-mesnil
Copy link

Hi, can we merge this soon? We can't use eslint 9 for now. Thanks 🙏🏻

@mshima
Copy link

mshima commented Nov 15, 2024

2. Create a new package eslint-config-react-native-flat and publish it so other projects can use the new config format.

My suggestion is to create a new package called @react-native/eslint.
It’s not really a plugin, it’s a pre-defined config recommendation.

@YowaiCoder
Copy link

It's been almost a year, guess we‘ll never get there. 😂

Comment on lines +21 to +32
const reactNative = require('@react-native/eslint-plugin');
const myplugin = require('my-plugin');

module.exports = [
...reactNative.configs.flat,
{
plugins: {
myplugin
},
...
},
];
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this example use ESM?

@kraenhansen
Copy link
Contributor

I would love to see this PR get some more 💙 and attention.

@MCanhisares I know it's been a while, but I have a few clarifying questions towards your last comment.

In order for this to work though, we need to change our approach for the configs, and export one that supports legacy .eslintrc projects and one for projects

Does the PR in it's current state reflect this? Or is that an outstanding task?

Another problem arises from this, where eslint reads the package name to create an alias to the config itself,

When is this an issue?

the tests might need to be updated to correctly read the new eslint configs

Do you need any help with this?

@react-native-bot
Copy link
Collaborator

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@react-native-bot react-native-bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Oct 9, 2025
@react-native-bot react-native-bot removed the Stale There has been a lack of activity on this issue and it may be closed soon. label Oct 9, 2025
@pipopotamasu
Copy link
Contributor

I created the PR for Flat Config support because this PR is stale and doesn't seem likely to move forward

#54297

@MCanhisares
Copy link
Contributor Author

I created the PR for Flat Config support because this PR is stale and doesn't seem likely to move forward

#54297

Glad you were able to figure out a solution and the maintainers responded to the PR - as soon as it's merged I'll close this one

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

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.