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 @reach/auto-id issue by removing the dependency #1484

Merged
merged 3 commits into from
Jul 18, 2022
Merged

Conversation

gpbl
Copy link
Owner

@gpbl gpbl commented Jul 17, 2022

Context

We use the good @reach/auto-id package for assigning a unique id to the month captions. This package is creating warnings with DayPicker and React 18 (e.g. #1478), and with React 17 (reach/reach-ui#921).

Analysis

The source of the package includes some clever code to support both React 17 and the native's useId from React 18. However, it doesn't play well in the context of a redistributed package like DayPicker.

Solution

I copied in src the useId source code. Skipped the lines of code that would use React 18's useId.

With this, DayPicker will fallback to the useEffect solution – even on React 18. We hopefully will find a better solution in the future.

Discarded options:

  • use accessible/use-id - a fork of auto-id. Doesn't seem a much better solution at the burden of maintaining an extra dependency
  • developing my own useId (no time for this, and I'd end up reinventing the wheel)
  • make React 18+ mandatory to use the new useId hook - this would make lot of users unhappy as react 16 and 17 are still widely used
  • keep the warning in React 18, but this wouldn't fix @reach/auto-id y React18 #1478

@gpbl gpbl merged commit 1441c66 into master Jul 18, 2022
@gpbl gpbl deleted the gpbl/issue1478 branch July 18, 2022 00:47
@dartess
Copy link

dartess commented Aug 25, 2022

Oh my god, I spent several days in preparatory work to fix @reach/auto-id, I already set them up to run tests on multiple versions of React and provided support for react@18 in @reach/utils which is used by @reach/auto-id.
And now I happen to be here (most likely).
And now I don't have to worry about it.
Strange feeling. I just contributed to a package that I have never used and never will use.
Anyway, thanks for your work.

@gpbl
Copy link
Owner Author

gpbl commented Aug 25, 2022

👋 @dartess thanks for writing here! Not sure I understand - did you commit to @reach/auto-id to fix the issue exposed here?

I would love to use that package for react-day-picker – maybe with your update this will be fixed? Do you have a link to your work?

@dartess
Copy link

dartess commented Aug 26, 2022

@gpbl I just planned to do it.
But @reach/auto-id depends on package @reach/utils, which is also not compatible with React 18.
And for tests in React 18, the test environment was not configured in the repository.

So, I first set up the tests: reach/reach-ui#959
Then I worked on @reach/utils, write test and check compatible with React 18: reach/reach-ui#960
After that, I planned to study the problems of @reach/auto-id.

If this is still relevant for your package, I may try continue this (It seems like it's not really needed).

Since we're talking about it here, you said this:

However, it doesn't play well in the context of a redistributed package like DayPicker.

Can you elaborate on what you mean? I'll try to keep that in mind.

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.

@reach/auto-id y React18
2 participants