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

fix: simplify ESLint config to improve dependency management #2883

Merged

Conversation

coolsoftwaretyler
Copy link
Contributor

Please verify the following:

  • yarn test jest tests pass with new tests, if relevant
  • yarn lint eslint checks pass with new code, if relevant
  • yarn format:check prettier checks pass with new code, if relevant
  • README.md (or relevant documentation) has been updated with your changes
  • If this affects functionality there aren't tests for, I manually tested it, including by generating a new app locally if needed (see docs).

Describe your PR

In #2823, we added --legacy-peer-deps to the npm install script in order to try and resolve #2818 (issues with conflicting eslint-plugin-n and eslint-config-standard).

I believe this led to some new bugs because of peer dependency resolution in npm, like #2840. We have also had some reports in Slack where people are getting confused because they need to use the --legacy-peer-deps flag on subsequent dependency installs.

I was talking to @frankcalise yesterday about these plugins. They've been in Ignite for a while, and maybe it's simpler to eliminate eslint-plugin-n at this point. As far as I can tell, these are just extra rules for Node.js, which might not be very relevant in most React Native projects.

There was some support for slimming down linting configuration in this Slack thread from October. So I hope this PR aligns with that approach. I think it's reasonable to remove ESLint rules for Node.js in order to simplify a React Native project.

Alternative approach

If we want to keep using the standard config and the eslint-plugin-n, we need to use neostandard, which requires ESLint v9. But we are blocked on adopting ESLint v9 until React Native settles on support for ESLint v9: facebook/react-native#43959, but that isn't ready yet.

Screenshots (if applicable)

No screenshots since there are no real UI changes, but here are two Ignite commands I ran from my local branch:

Standard settings, but use npm as the packager (just testing this PR as a replacement fix for #2823)

   npx ignite-cli new npmfix \
        --bundle=com.npmfix \
        --git \
        --install-deps \
        --packager=npm \
        --target-path=/Users/tylerwilliams/npmfix \
        --remove-demo=false \
        --new-arch=false \
        --workflow=cng \
        --no-timeout=false \
        --state=mst

Standard settings, but use npm as the packager, and enable Expo Router (fixes #2840, which is related to expo/expo#29150)

      npx ignite-cli new npmfixexporouter \
        --bundle=com.npmfixexporouter \
        --git \
        --install-deps \
        --packager=npm \
        --target-path=/Users/tylerwilliams/npmfixexporouter \
        --remove-demo=false \
        --new-arch=false \
        --experimental=expo-router \
        --workflow=cng \
        --no-timeout=false \
        --state=mst

In both cases I ran npm run android and npm run ios from the project and Ignite started without issue.

Failing with Yarn, now:

Now if I try to use yarn as my package manager, I end up with a new error after running npx ignite-cli new yarncheck --yes:

The following error occurred:

    yarn run v1.22.22
$ eslint . --fix

Oops! Something went wrong! :(

ESLint: 8.57.1

ESLint couldn't find the plugin "eslint-plugin-n".

(The package "eslint-plugin-n" was not found when loaded as a Node module from the directory "/Users/tylerwilliams/yarncheck".)

It's likely that the plugin isn't installed correctly. Try reinstalling by running the following:

    npm install eslint-plugin-n@latest --save-dev

The plugin "eslint-plugin-n" was referenced from the config file in ".eslintrc.js » eslint-config-standard".

If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team.

error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

@coolsoftwaretyler coolsoftwaretyler changed the title fix: drop eslint plugin n to simplify dependencies draft: drop eslint plugin n to simplify dependencies Jan 11, 2025
@coolsoftwaretyler
Copy link
Contributor Author

The new yarn failure is what's failing CI.

@coolsoftwaretyler
Copy link
Contributor Author

Talked about this more in Slack - if we're open to dropping eslint-config-standard and eslint-plugin-promise, then this PR will work. I ran this in four configurations:

  1. All default (yarn, no expo router)
  2. Yarn and expo router
  3. npm and no expo router
  4. npm and expo router

In each case, the initial Ignite command worked, along with the lint command.

@coolsoftwaretyler coolsoftwaretyler changed the title draft: drop eslint plugin n to simplify dependencies fix: simplify ESLint config to improve dependency management Jan 12, 2025
Copy link
Contributor

@frankcalise frankcalise left a comment

Choose a reason for hiding this comment

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

This looks good to me. I know you didn't modify it but mind removing the format script from package.json in boilerplate? It seems to be a duplicate of lint. Since we have lint:check seems reasonable to keep lint over format

@coolsoftwaretyler coolsoftwaretyler merged commit 2ab0092 into master Jan 12, 2025
1 check passed
@coolsoftwaretyler coolsoftwaretyler deleted the fix/drop-problematic-eslint-config-and-plgin branch January 12, 2025 19:16
infinitered-circleci pushed a commit that referenced this pull request Jan 12, 2025
## [10.1.3](v10.1.2...v10.1.3) (2025-01-12)

### Bug Fixes

* **boilerplate:** simplify ESLint config ([#2883](#2883) by [@coolsoftwaretyler](https://github.com/coolsoftwaretyler)) ([2ab0092](2ab0092))
@infinitered-circleci
Copy link

🎉 This PR is included in version 10.1.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

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