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(type): clarify the types are drived from the Agnostic types #12051

Closed

Conversation

nightohl
Copy link

@nightohl nightohl commented Oct 1, 2024

Changes

Hi there! It's my first contribution to this project :)

I understand that Agnostic is typed to be used universally, and the types are added and modified after inheriting it to fit the React environment.

At first, I was curious about the types, but as I looked through the list, I found it challenging to understand concepts such as whether they inherit from Agnostic, how much of the original Agnostic is carried over, whether all elements of Agnostic are included or only some are borrowed, and the differences between Index and NonIndex. Of course, my lack of background knowledge contributes to this difficulty.

Considering this, I thought that if it were clear in the types that they are defined after inheriting from Agnostic to fit the environment, it would be much easier to understand. So, I made some modifications based on that idea!

IndexRouteObject, NonIndexRouteObject

image

PathRouteProps, IndexPathRouteProps

image

Test

image

Copy link

changeset-bot bot commented Oct 1, 2024

⚠️ No Changeset found

Latest commit: 51930d1

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@timdorr
Copy link
Member

timdorr commented Oct 1, 2024

Thanks, but while this might make more sense from a duplication standpoint in the codebase, it would probably make interpreting the types a little harder for end-users. We'll leave this as-is.

@timdorr timdorr closed this Oct 1, 2024
@nightohl
Copy link
Author

nightohl commented Oct 1, 2024

@timdorr
First of all, thank you for the quick review.

My motivation was to make the code easier to understand and to represent the concept more explicitly.

These types aren’t inherently complex. The idea is simple "bringing over the Agnostic concept, which isn’t tied to any environment, with some React-specific code added." However, I thought the current type doesn't clearly convey this concepts, which made it harder to understand.

I believe a pseudocode like this would make it clear without further explanation:

ReactSpecificType extends Agnostic {
   // types added/modified for React environment
}

Points of confusion I encountered:

  1. A lot is pulled from Agnostic, but it’s not inheritance—just referencing some properties?
  2. If then, what’s been brought over, and what hasn’t?
  3. Why redefine index and children instead of using Agnostic['index'] and Agnostic['children']? Is there a small change?
  4. What parts are from Agnostic, and what’s added for React? (The redefined index and children which are already present in Agnostic, made this unclear.)
  5. React-specific code seems identical but is declared each time. Are there slight differences among Index and NonIndex or more?

After comparing the types in detail, I realized that clearer intent in the code would have made this easier to grasp.
This refactor clarifies the concept without extra comments, and grouping shared types improves maintainability also.

As someone expanding from React Native to React, I'm not familiar to React Router yet, so I was trying to understand its types and faced these challenges.

Even if this PR isn’t merged, it's okay since I now understand about the types and concepts.
But if you relate to my confusion, reconsidering the merge would be appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants