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: typescript paths #2182

Merged
merged 2 commits into from
Jun 14, 2024
Merged

fix: typescript paths #2182

merged 2 commits into from
Jun 14, 2024

Conversation

alduzy
Copy link
Member

@alduzy alduzy commented Jun 12, 2024

Description

This PR fixes the errors showing on yarn prepare command. Turned out that custom path definition to react-navigation folder produced typescript errors when running the command. The default node_modules directory contains typescript definitions that do not cause any problems.
Some of the react-navigation packages were missing in node modules and the ts support was missing in examples so I added them as devDependencies - now there are no errors during the prepare command and there is full TS support in common examples directory.

The alternative approach would be to define compilerOptions paths pointing to react-navigation folder inside examples and test-examples tsconfigs individually instead of adding new devDependencies

Edit: decided to go with the second approach as it does not add any extra dependencies - the tsconfigs inside common example directories redefine paths.

Changes

  • removed custom paths to react-navigation types (node_modules is the default)
  • added the paths to react-navigation types individually in apps/examples and apps/test-examples

Test code and steps to reproduce

Checklist

@alduzy alduzy requested review from WoLewicki and kkafar June 12, 2024 13:51
Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

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

Let's make sure it works now 😅 Please test by running yarn prepare, runnning apps, looking for errors in code editor (we don't want "red" back.

Let me know ☝🏻 how does it look

@alduzy
Copy link
Member Author

alduzy commented Jun 13, 2024

@kkafar Now the yarn prepare command works as expected without any errors with typescript path pointing to node_modules/@react-navigation (default).
The custom path pointing to react-navigation folder is now only used inside example dirs to provide ts support in example files and it works as intended 🙂.

@alduzy alduzy requested a review from kkafar June 13, 2024 08:24
Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

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

I guess this is how this should work, because inside our native stack implementation (v5) we still use old types from react-navigation v5, thus it does not make sense to use types from our submodule. The goal of the submodule is to have easy time modifying v6 / v7 native stack code within our examples.

The changes look fine, make sure this works fine with code editors & our example do work just fine and we're good to go.

@alduzy alduzy merged commit ea85934 into main Jun 14, 2024
1 check passed
@alduzy alduzy deleted the @alduzy/ts-config-fix branch June 14, 2024 08:17
ja1ns pushed a commit to WiseOwlTech/react-native-screens that referenced this pull request Oct 9, 2024
## Description

This PR fixes the errors showing on `yarn prepare` command. Turned out
that custom path definition to react-navigation folder produced
typescript errors when running the command. The default node_modules
directory contains typescript definitions that do not cause any
problems.
~~Some of the react-navigation packages were missing in node modules and
the ts support was missing in examples so I added them as
devDependencies - now there are no errors during the prepare command and
there is full TS support in common examples directory.~~

~~The alternative approach would be to define compilerOptions paths
pointing to react-navigation folder inside examples and test-examples
tsconfigs individually instead of adding new devDependencies~~

Edit: decided to go with the second approach as it does not add any
extra dependencies - the tsconfigs inside common example directories
redefine paths.

## Changes

- removed custom paths to react-navigation types (node_modules is the
default)
- added the paths to react-navigation types individually in
apps/examples and apps/test-examples

<!--

## Screenshots / GIFs

Here you can add screenshots / GIFs documenting your change.

You can add before / after section if you're changing some behavior.

### Before

### After

-->

## Test code and steps to reproduce

<!--
Please include code that can be used to test this change and short
description how this example should work.
This snippet should be as minimal as possible and ready to be pasted
into editor (don't exclude exports or remove "not important" parts of
reproduction example)
-->

## Checklist

- [ ] Included code example that can be used to test this change
- [ ] Updated TS types
- [ ] Updated documentation: <!-- For adding new props to native-stack
-->
- [ ]
https://github.com/software-mansion/react-native-screens/blob/main/guides/GUIDE_FOR_LIBRARY_AUTHORS.md
- [ ]
https://github.com/software-mansion/react-native-screens/blob/main/native-stack/README.md
- [ ]
https://github.com/software-mansion/react-native-screens/blob/main/src/types.tsx
- [ ]
https://github.com/software-mansion/react-native-screens/blob/main/src/native-stack/types.tsx
- [ ] Ensured that CI passes
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.

2 participants