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

chore: update contributing guide #2140

Merged
merged 33 commits into from
Jun 11, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
a1542d6
chore: update contributing guide
maciekstosio May 17, 2024
eac922d
Update guides/CONTRIBUTING.md
maciekstosio May 17, 2024
6affe06
Update guides/CONTRIBUTING.md
maciekstosio May 17, 2024
88a8708
Update guides/CONTRIBUTING.md
maciekstosio May 17, 2024
369c0b4
Update guides/CONTRIBUTING.md
maciekstosio May 17, 2024
493af4d
Update guides/CONTRIBUTING.md
maciekstosio May 17, 2024
a853562
Update guides/CONTRIBUTING.md
maciekstosio May 17, 2024
1d6505e
Update guides/CONTRIBUTING.md
maciekstosio May 17, 2024
e460e53
Update guides/CONTRIBUTING.md
maciekstosio May 17, 2024
4bb3d6a
Update guides/CONTRIBUTING.md
maciekstosio May 21, 2024
19d5dff
Update guides/CONTRIBUTING.md
maciekstosio May 21, 2024
333cced
Update guides/CONTRIBUTING.md
maciekstosio May 21, 2024
f53024a
Update guides/CONTRIBUTING.md
maciekstosio May 21, 2024
cd29b95
Update guides/CONTRIBUTING.md
maciekstosio May 21, 2024
26c79a2
Update guides/CONTRIBUTING.md
maciekstosio May 21, 2024
ebde0a8
Update guides/CONTRIBUTING.md
maciekstosio May 21, 2024
f0e9281
Update guides/CONTRIBUTING.md
maciekstosio May 21, 2024
f30b3fe
Update guides/CONTRIBUTING.md
maciekstosio May 21, 2024
2f18740
Update guides/CONTRIBUTING.md
maciekstosio May 21, 2024
c86c3e5
Update guides/CONTRIBUTING.md
maciekstosio May 21, 2024
366212b
Update guides/CONTRIBUTING.md
maciekstosio May 21, 2024
8a3d2f4
fix: add changes from comments
maciekstosio May 21, 2024
08153b2
fix: add changes based on components
maciekstosio May 21, 2024
66516d3
chore: update docs after unification of examples and test-examples
maciekstosio May 28, 2024
b4ed2ce
chore: add changes from comments
maciekstosio Jun 3, 2024
4e7c2cf
chore: update docs according to PR comments
maciekstosio Jun 4, 2024
54c3e6d
chore: update native-stack description
maciekstosio Jun 4, 2024
596eb79
Update CONTRIBUTING.md
maciekstosio Jun 4, 2024
ab83052
Update guides/CONTRIBUTING.md
maciekstosio Jun 11, 2024
c51c184
Update guides/CONTRIBUTING.md
maciekstosio Jun 11, 2024
b82d019
Update guides/CONTRIBUTING.md
maciekstosio Jun 11, 2024
078b435
Update guides/CONTRIBUTING.md
maciekstosio Jun 11, 2024
34c7abc
Update guides/CONTRIBUTING.md
maciekstosio Jun 11, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
156 changes: 143 additions & 13 deletions guides/CONTRIBUTING.md
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
# Contributing to `react-native-screens`
# Contributing to react-native-screens

### Welcome
## Welcome!

Thank you for considering contributing to `react-native-screens`. It's people like you that make open source projects thrive! We love to receive contributions from our community and there are many ways for *you* to be a part of this.

_tl;dr **Please** do not ignore templates for issues and pull requests and everything will be fine_

### Found a bug?

If you’ve encountered a bug, don't hesitate to submit [an issue](https://github.com/software-mansion/react-native-screens/issues). Just check if someone didn't report it lately.
Expand All @@ -18,26 +16,158 @@ When filing an issue, make sure to provide:
- **snippet of code or Snack that reproduces the bug**. Check out [this guide](https://stackoverflow.com/help/minimal-reproducible-example)
- version of `react-native` and `react-native-screens`

### Got a question or an idea for a feature?

We use GitHub issues exclusively for tracking bugs. For questions and feature requests check out [Discussions](https://github.com/software-mansion/react-native-screens/discussions).

We've provided a template on GitHub that simplifies the process of filing an issue.

Following these few steps will show great respect for the time of the developers managing and developing this open-source project.

We inform you that unrespectful issues will be closed.

### Got a question or an idea for a feature?
## Ways to Contribute

We use GitHub issues exclusively for tracking bugs. For questions and feature requests check out [Discussions](https://github.com/software-mansion/react-native-screens/discussions).
1. **Replying and handling open issues** – great way to contribute without writing a single line of code is triaging the issues. We often get issues that have generic errors, occur only in very specific cases, do not have proper example or reproducible repository. One of they way to help is preparing and filling those details, which will help other contributors get up to speed with the issue faster.

maciekstosio marked this conversation as resolved.
Show resolved Hide resolved
2. **Reviewing pull requests** – reviewing Pull Requests is crucial as it may help catch the corner cases or bugs that the developer did not notice. Every review matters as it may help polish quality of the library.

3. **Contributing to Code** – code-level contributions generally come in the form of pull requests. By contributing to code you help us with solving issues, fixing bugs or introducing new amazing features. If you want to start your adventure with open source it's good idea to take a look at [good first issue](https://github.com/software-mansion/react-native-screens/pulls?q=is%3Apr+is%3Aopen+label%3A%22good+first+issue%22) on github. Read more about [contributing to code](#contributing-to-code).

maciekstosio marked this conversation as resolved.
Show resolved Hide resolved
### Repository overview

- `android` – source code of Android native implementation
- `common` – C++ code related to components - shadow nodes and state
maciekstosio marked this conversation as resolved.
Show resolved Hide resolved
- `cpp` – C++ code for turbo modules
- `Example` – paper version of React Native mobile example app
- `FabricExample` – fabric version of React Native mobile example app
- `FabricTestExample` – fabric version of React Native mobile app containing test examples
- `gesture-handler` – interop between react-native-screens and react-native-gesture-handler
- `guides` – guides for developers
- `ios` – source code of iOS native implementation
- `native-stack` – native stack v5, this will be deprecated in favor of react-navigation in near feature
maciekstosio marked this conversation as resolved.
Show resolved Hide resolved
- `react-navigation` – git submodule containing react-navigation
maciekstosio marked this conversation as resolved.
Show resolved Hide resolved
- `reanimated` – interop between react-native-screens and react-native-reanimated
maciekstosio marked this conversation as resolved.
Show resolved Hide resolved
- `scripts` – cli utils
- `src` – JS core code
maciekstosio marked this conversation as resolved.
Show resolved Hide resolved
- `TestsExample` – paper version of React Native mobile app containing test examples
- `TVOSExample` – React Native for TVOS app wrapper for shared example code
- `windows` – react-native-screens support for Windows

maciekstosio marked this conversation as resolved.
Show resolved Hide resolved
## Handling open issues

Often understanding and reproducing the problem can be very time consuming task. The great way to help other contributors get up to the speed with solving an issue is providing detailed description and *reproducible* example. The github already has an template for creating issue, nevertheless we still encounter issues that do not have all necessary details, like:

maciekstosio marked this conversation as resolved.
Show resolved Hide resolved
- repository that we can clone and quickly see the problem,
- very generic reproduction steps,
- missing description,
- not related or truncated stack trace.

maciekstosio marked this conversation as resolved.
Show resolved Hide resolved
What you can do is ask the owner for those details or try provide them by yourself - try to reproduce the problem and provide missing details, so other developer can start debugging straightaway! 🎉

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
What you can do is ask the owner for those details or try provide them by yourself - try to reproduce the problem and provide missing details, so other developer can start debugging straightaway! 🎉
Before filling the issue, you should consult the owner for such details or try providing them by yourself - try to reproduce the problem and provide missing details, so that other developers could start debugging straightaway! 🎉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was no the point here. My idea is that other contributors can discuss the problem in the comments to fill out missing details, so we have less work with reproduction, not creating another issue.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, let's do this then 👍

Suggested change
What you can do is ask the owner for those details or try provide them by yourself - try to reproduce the problem and provide missing details, so other developer can start debugging straightaway! 🎉
What you can do is ask the owner of an issue for such details or try providing them by yourself - try to reproduce the problem and provide missing details, so that other developers could start debugging straightaway! 🎉

## Contributing to Code

Posting Pull Requests to the issues is great way to contribute to Reanimated. If you eager to start contributing right away, we have list of [good first issue](https://github.com/software-mansion/react-native-screens/pulls?q=is%3Apr+is%3Aopen+label%3A%22good+first+issue%22) that contain bugs which have limited scope. In this section we'll describe in more details how to play around with react-native-screens setup.

maciekstosio marked this conversation as resolved.
Show resolved Hide resolved
> [!tip]
> For commits and pull request names we follow a [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) specification.

We have two types of applications: Example (Example, FabricExample) and TestsExample (FabricTestExample, TestsExample), the first work as a showcase of the library, the latter contains specific test cases that corresponds to github issues. For example Test1864.tsx corresponds to issue [#1864](https://github.com/software-mansion/react-native-screens/issues/1864). Our developer flow usually consists of creating new `Test*.ts` with code example that we try to fix or add. For the new features we try to prepare showcases in Example app.

maciekstosio marked this conversation as resolved.
Show resolved Hide resolved
- `Example/src` – source code with showcase app for paper architecture
- `TestsExample/src` – source code with tests example app for paper architecture
- `FabricExample/src` – source code with showcase app for fabric architecture
- `FabricTestExample/src` – source code with tests example app for fabric architecture
- `TVOSExample/src` – source code with example app for TVOS
- `src` – contains JS core code
- `android` – source code related to Android native part
- `ios` – source code related to iOS native part

Copy link
Member

Choose a reason for hiding this comment

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

I don't know if that's necessary (at least 82-84 lines)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's pretty self-explanatory but I wanted to list all important places for the new contributor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kkafar let you know what's your take at it

### Working on Android

To start with, let install all dependencies:

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
To start with, let install all dependencies:
To begin with working on Android implementation, let's install all dependencies:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something doesn't sound right after those changes. I think I leave it as it was was, just change the let to let's

1. `yarn`
2. `yarn submodules`
3. `cd TestsExample`
maciekstosio marked this conversation as resolved.
Show resolved Hide resolved
4. `yarn`
5. `yarn start` – make sure to start metro bundler before building the app in Android Studio

and open `react-native-screens/TestsExample/android` with Android Studio.

![Android Studio](android_studio.png)

The native source code of react-native-screens can be found in `react-native-screens` module, in `kotlin+java/com.swmansion.rnscreens`. Making sure metro builder is run, you can now build React Native app or debug native code.

maciekstosio marked this conversation as resolved.
Show resolved Hide resolved
### Working on iOS

To start with, let install all dependencies:

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
To start with, let install all dependencies:
To begin with working on iOS implementation, let's install all dependencies:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something doesn't sound right after those changes. I think I leave it as it was was, just change the let to let's

1. `yarn`
2. `yarn submodules`
3. `cd TestsExample`
maciekstosio marked this conversation as resolved.
Show resolved Hide resolved
4. `yarn`
5. `(cd ios && pod install)`
6. `yarn start` – make sure to start metro bundler before building the app in XCode.

and open `react-native-screens/TestsExample/ios/TestsExample.xcworkspace` with XCode.

![XCode](xcode.png)

To find the native source code of Reanimated navigate to `Pods > Development Pods > RNScreens > TestsExample > node_modules > react-native-screens > ios`. Making sure metro builder is run, you can now build React Native app or debug native code.

maciekstosio marked this conversation as resolved.
Show resolved Hide resolved
### Fabric

When using fabric on iOS codegen is run while doing `pod install`, on android when running Fabric Example in Android Studio. Alternative approach for Android is to go into `./FabricExample/android` folder and run `./gradlew generateCodegenArtifactsFromSchema`. Next you need to copy changed files from `/android/build/generated/source/codegen/java/com/facebook/react/viewmanagers/` to `android/src/paper/java/com/facebook/react/viewmanagers/`, so the interfaces are in sync.

Copy link
Member

Choose a reason for hiding this comment

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

I would rewrite that paragraph, to be honest. It's good in substance, but I don't like the grammar there.

maciekstosio marked this conversation as resolved.
Show resolved Hide resolved
### Preparing Pull Request

When your code changes are ready, it is time to open your Pull Request. Github already has a template that helps you properly post your changes. The most crucial are:

maciekstosio marked this conversation as resolved.
Show resolved Hide resolved
1. **Description**:
- If you're solving specific issue, start with linking it.
- Write what are your motivations.
2. **Changes** - write what you have changes and why.
3. **Screenshots / GIFs** – if applicable it's great idea to attach screen or video before and after changes.
4. **Test code and steps to reproduce** – describe how others can test your change, if you didn't add `Test*.tsx` file it's good idea to add code snippets here.

### Do I need to prepare Pull Request for react-navigation too?

Currently native stack is both in `react-native-screens` and `react-navigation`. `react-native-screens` contains native-stack v5 (`/native-stack`), newer versions (v6, v7) are moved to `react-navigation`, hance in some cases it is necessary to prepare pull request for `react-navigation` alongside the `react-native-screens` changes.

maciekstosio marked this conversation as resolved.
Show resolved Hide resolved
> [!CAUTION]
> Currently, in this setup we have some code duplications, we planing soon to deprecate native-stack v5 and remove it from react-native-screens

maciekstosio marked this conversation as resolved.
Show resolved Hide resolved
#### Changes in native code

If your change is only related to native code, for example you're fixing a bug, you **do not need** to post PR to `react-navigation`. `react-native-screens` is separate library installed alongside `react-navigation`, you can just bump `react-native-screens` version in you package.json file.

maciekstosio marked this conversation as resolved.
Show resolved Hide resolved
#### Changes in JS code or both

If you're changing native-stack v5 (`./native-stack`) you **do not need** to post PR to `react-navigation`.

If you're changing native-stack v6, v7 you **need to** post PR to `react-navigation`, as the code belongs there now.

maciekstosio marked this conversation as resolved.
Show resolved Hide resolved
### Pull requests
If you're changing core functionality:
- If it's a bug fix and does not change the interface then you **do not need** to post PR to `react-navigation`.
- If you're adding new feature or changing API you **need to** post PR to `react-navigation`.

maciekstosio marked this conversation as resolved.
Show resolved Hide resolved
Pull requests are more than welcome 🚀
> [!TIP]
> As the rule of thumb, if you're changing the external interface you need to prepare PRs for react-navigation

maciekstosio marked this conversation as resolved.
Show resolved Hide resolved
We've created a template to help you with the process of submitting a pull request to this project. Remember to change or add documentation as needed and provide a code snippet (preferably added to `TestsExample/` project) that the developers could test your change on. In the PR template we've added a checklist for all the places where the documentation should be updated.
### What is the flow to integrate with react-navigation

If it's your first pull request [this guide](https://github.com/MarcDiethelm/contributing/blob/master/README.md) might be helpful for you.
When you propose the changes that require posting PR to react-navigation. Please follow those steps:

maciekstosio marked this conversation as resolved.
Show resolved Hide resolved
### Naming convensions
1. Create PR with `react-native-screens` changes
2. Create PR that changes the native-stack in `react-navigation`
3. Wait for both to pass the review and have all all checks passed
4. Merge `react-native-screens` changes
5. Merge `react-native-navigation` changes
6. Bump the version of main branch index ref in `react-native-screens`
maciekstosio marked this conversation as resolved.
Show resolved Hide resolved
7. Post and merge ASAP the main branch ref bump of `react-navigation`

maciekstosio marked this conversation as resolved.
Show resolved Hide resolved
For commits and pull request names we follow a [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) specification.
> [!WARNING]
> Those steps are crucial, if you change the API in react-native-screens and won't merge react-navigation changes the libraries may go out of sync and crash i.e. because of not existing property. On the other hand, if you don't perform step 6 and 7, test examples and showcase app may stop working in react-native-screens.

maciekstosio marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit exaggerated. Any backward compatible changes in react-native-screens, so changes in minor / patch versions should go unnoticed if react-navigation is not updated. Only breaking changes would break the behaviour and we're fine with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So my idea was that the previous sections let you know if you need or do not need to open PR to react navigation. And if you need, you end up in this section. Then those steps are crucial (?). But I can rephrase that if it's not clear.

For branch names we use `@username/description-separated-by-dashes` convention.
See you on GitHub! 🎉
maciekstosio marked this conversation as resolved.
Show resolved Hide resolved
Binary file added guides/android_studio.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added guides/xcode.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading