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(generate): add support for Expo Router app directory prompts #2905

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

cdanwards
Copy link
Member

@cdanwards cdanwards commented Feb 24, 2025

Description

This PR addresses a user who creates screens within an ignited app using expo-router. It checks for the existence of src/app (the expo router configuration) and then determines where to put the screen in 4 ways:

  • An override provided in the CLI command
  • destinationDir set from the screen generator template
  • If neither of those exists, it prompts the user to provide a dir path, if none is provided or they decide to not create that directory, it defaults to src/app/

Screenshots

Scenario Result
Using CLI Override for directory override
Using directory set in template front matter frontmatter
Setting via prompts - chooses to create dir created_dir
Setting via prompts - goes to default src/app Screenshot 2025-02-27 at 10 57 11 AM

Checklist

  • README.md and other relevant documentation has been updated with my changes
  • I have manually tested this, including by generating a new app locally (see docs).

@cdanwards cdanwards marked this pull request as ready for review February 27, 2025 16:04
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.

Nice work! A few comments for discussion where I think we can shore things up. Also if you think we can add a test to double check the user gets prompted in the way you're expecting if both are omitted that's probably worth adding.

@@ -52,6 +52,53 @@ async function generate(toolbox: GluegunToolbox) {
pascalName = pascalName.slice(0, -1 * pascalGenerator.length)
command(`npx ignite-cli generate ${generator} ${pascalName}`)
}
// Check if src/app exists, denoting an Expo Router app
if (generator === "screen") {
const isExpoRouterApp = filesystem.exists("src/app") === "dir"
Copy link
Contributor

Choose a reason for hiding this comment

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

So this is the correct directory structure to determine expo-router in an Ignite application, however omitting the src/ nesting is totally valid and it is possible a dir structure looks like this:

  • app (the routes)
  • components
  • hooks
  • etc...

This is in fact the default expo application structure and Ignite generators should continue to be compatible for any project, not just our opinionated structure.

So perhaps we should look into a different mechanism for determining this? Maybe we check app or src/app to cover both. Or look for package.json to have expo-router?

Comment on lines +374 to +377
if (frontMatterData?.destinationDir === undefined) {
warning(`⚠️ No front matter found in ${generator} template.`)
return undefined
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we always want to warn here? I maybe only want to see this if I haven't provided --dir because if that's my workflow and I never set it in the template, it's going to show each time.

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.

Generating a new screen when using Expo Router results in wrong placement
2 participants