-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Fixes for expo-router app generation and issues with master merge up #2772
Conversation
@frankcalise I'm getting this type error in I'm not sure what the desired interface here looks like to reconcile this and still work for expo-router. Do you know how you'd like to fix this? |
675ff0b
to
59ff2a3
Compare
@fpena I think I've fixed every reproducible error I can find, check out this branch and let me know how it goes for you |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just left 1 non-blocking question.
"src/devtools/ReactotronConfig.ts", | ||
"src/components/Toggle/Switch.tsx", | ||
"src/components/ListView.tsx", | ||
"test/setup.ts", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious: Why are we removing the 3 lines on the left?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the alias was updated so there are less specific changes to files that are necessary. Before the changes were very specific and didn't scale well, now @lindboe has adjusted the path alias in tsconfig
and we don't have to do so many transformations across the source files (whether they're in app/
or src/
) - less maintenance for us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition, the test files are still in there because there's a separate TS config, but if we can unify those better we can also update the test files automatically. But I'm planning on circling back to that in a later change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much cleaner and looks to be more future proof, well done!
"src/devtools/ReactotronConfig.ts", | ||
"src/components/Toggle/Switch.tsx", | ||
"src/components/ListView.tsx", | ||
"test/setup.ts", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the alias was updated so there are less specific changes to files that are necessary. Before the changes were very specific and didn't scale well, now @lindboe has adjusted the path alias in tsconfig
and we don't have to do so many transformations across the source files (whether they're in app/
or src/
) - less maintenance for us.
These lines weren't getting removed in demo and failed tests. Did we not have a test for removing demo and compiling?
I'm confused about this one, it appeared to work and printed `patches/patch.patch` originally, but now the path isn't including the directory. So added a new CWD to make sure the file gets deleted, otherwise bun fails.
...from the test it was copied from
5211d2f
to
63ea164
Compare
Potential issue with buffer size. Update to use spawn later so we can stream output.
63ea164
to
2dafde8
Compare
They added a bug, there's a fix but it's unreleased: jsx-eslint/eslint-plugin-react#3821
🎉 This PR is included in version 10.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Please verify the following:
yarn test
jest tests pass with new tests, if relevantyarn lint
eslint checks pass with new code, if relevantyarn format:check
prettier checks pass with new code, if relevantREADME.md
(or relevant documentation) has been updated with your changesDescribe your PR
We found some issues with paths when generating a new app with expo-router enabled. To make this easier to manage, I added
@
as an alias to consistently refer to the root app directory, whether it'sapp
orsrc
(depending on react-navigation vs. expo-router).This still requires us to transform some files, but much fewer. I also left test files out of the path alias configuration for now, because we have a separate test-tsconfig file and we don't immediately need to fix that, but I'd love to remove that separate configuration in the future.
I also found:
I'm concluding that it's extra important to make PRs for merging up changes from
master
at this point because v10 now has a lot more tests for making sure the generated app has no errors.