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: include ajv in devDependencies for Expo Router and npm #2842

Merged

Conversation

coolsoftwaretyler
Copy link
Contributor

@coolsoftwaretyler coolsoftwaretyler commented Nov 10, 2024

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

This would resolve one of the issues in #2840, although there are additional problems with the Expo Router template.

This is a little bit of a sledgehammer approach, but I wanted to start with the simple solution and we can enhance it if that seems necessary.

In #2823, we added the --legacy-peer-deps flag for npm install commands. This fixed a problem upstream from standard/eslint-config-standard#412, but I believe it has opened up a new problem for users who choose to use Expo Router and npm. Based on the issues that Expo Router + ESLint has experienced with how bun resolves peer dependencies, I am guessing the legacy peer dependency resolution falls victim to the same problem.

The very simplest way to solve this is if the generated Ignite project includes ajv at the right version number in its own devDependencies. This version "wins" across the board.

The drawback to this approach is that if Expo Router changes its required ajv version, there may be some new errors for the project to resolve.

Another drawback to this PR is that it would add ajv to all projects, including other package managers that don't suffer from this problem, and including projects that do not opt in to Expo Router. So we could alternatively run npm install ajv@^8 --legacy-peer-deps in the install command right after

return `npm install${silent} --legacy-peer-deps`
, and maybe even do a check for if Expo Router is enabled.

Screenshots (if applicable)

No screenshots, but running node ~/ignite/bin/ignite new project-name and choosing npm and Expo Router successfully works with this change.

@coolsoftwaretyler
Copy link
Contributor Author

16e30eb is the more targeted approach, but we can easily drop that commit if simple is preferred.

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.

Thanks for this! I think being more specific here is fine with the npm+router check

Does something need a proper upstream fix somewhere?

@coolsoftwaretyler
Copy link
Contributor Author

Yeah, eslint-plugin-n needs to fix their peer dependencies, as in standard/eslint-config-standard#412 (this is why we added --legacy-peer-deps for our own issue over in #2818). Once eslint-plugin-n resolves their issue, we should be able to remove --legacy-peer-deps from our commands here and then remove this patch as well.

@frankcalise
Copy link
Contributor

@coolsoftwaretyler ah yeah I had a discussion about this in Slack. config-standard is abandoned I think for neostandard in that issue thread https://infinitered.slack.com/archives/C0X0P5LP7/p1730325079888599

should we look to swap it out for neo and see if n just works or cut back on all these extra config / plugins?

@coolsoftwaretyler
Copy link
Contributor Author

@frankcalise - if we swap in neostandard and it Just Works, I think that's ideal. I can take a look sometime this week to make sure it doesn't cause any changes while linting a freshly ignited project.

Want to merge this one for now to patch up the ajv issue in the short term and I'll get back to ya in a few days on neostandard?

@coolsoftwaretyler coolsoftwaretyler merged commit 6baf76b into master Nov 13, 2024
1 check passed
@coolsoftwaretyler coolsoftwaretyler deleted the fix/2840-legacy-peer-deps-for-ajv-and-expo-router branch November 13, 2024 15:43
infinitered-circleci pushed a commit that referenced this pull request Nov 27, 2024
## [10.0.5](v10.0.4...v10.0.5) (2024-11-27)

### Bug Fixes

* **boilerplate:** remove interop definition for FlashList ([#2838](#2838) by [@frankcalise](https://github.com/frankcalise)) ([b368bcb](b368bcb))
* **boilerplate:** update app.tsx useEffect ([#2850](#2850) by [@objective](https://github.com/objective)See) ([2886487](2886487))
* **cli:** component generator template off in expo-router ([#2854](#2854) by [@frankcalise](https://github.com/frankcalise)) ([95f7642](95f7642))
* **cli:** include ajv in devDependencies for Expo Router and npm ([#2842](#2842) by [@coolsoftwaretyler](https://github.com/coolsoftwaretyler)) ([6baf76b](6baf76b))
@infinitered-circleci
Copy link

🎉 This PR is included in version 10.0.5 🎉

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