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

Rough pass at pulling out react specific utils into dedicated package #1819

Merged
merged 11 commits into from
Sep 5, 2022

Conversation

sarayourfriend
Copy link
Contributor

@sarayourfriend sarayourfriend commented Aug 31, 2022

Follow up from the related discussion (#1723) I've started working on this rough draft PR for how pulling out the React-specific utilities from the base ariakit-utils package might work.

Given the size of the PR I thought it would be appropriate to give some guidance on how to review it.

There should be essentially no actual changes internally speaking. There are major breaking changes to the API of ariakit-utils which made me question whether it would be necessary to create a major version change on that package... but give it's kind of in alpha/beta maybe that version guarantee is not really being enforced too closely for the packages? What do you think Diego?

The major change is to remove all references to React in ariakit-utils. The intention here is that ariakit-utils should be freely usable by any ariakit view-library implementation without pulling in types that will pollute the global namespace. @types/react and @vue/runtime-dom for example both modify the global JSX namespace that is used by TypeScript to type check JSX. The React and Vue types are incompatible on some basic levels, so this is a necessity if the two libraries should be supported in the future. I'm not familiar with Svelte or Angular as much, but I'm willing to bet they might have similar problems.

The newly introduced ariakit-react-utils package should be the place to house React related utilities. This includes the system functions and the React specific types. Vue would need its own bespoke implementation of all of these utilities (or at least the appropriate ones, tbh Vue's component model doesn't really fit how Ariakit manages React components) for them to make sense with Vue's setup model and so forth. Again, Svelte and Angular would also more than likely need their own versions of these.

As we uncover more API similarities there will probably be room for additional things to live inside ariakit-utils or maybe some intermediary ariakit-component-utils library that defines common APIs that each view library would adapt to. For now we just need to make sure that the basic utils are usable by all of them without the TypeScript conflict issues.

The rest of the changes aside from pulling the React utils into their own package are updating import names. I did most of this via some tricky find/replace operations. Thankfully TypeScript correctly catches errors here so I also used the type check to guide the final improvements here. I think if the type check passes the imports should be fine.

The tests of course should also pass without exception.

cc @visualjerk

@changeset-bot
Copy link

changeset-bot bot commented Aug 31, 2022

🦋 Changeset detected

Latest commit: df707f0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
ariakit-utils Minor
ariakit Patch
ariakit-playground Patch
ariakit-react-utils Patch
ariakit-test Patch

Not sure what this means? Click here to learn what changesets are.

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

@vercel
Copy link

vercel bot commented Aug 31, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
ariakit ✅ Ready (Inspect) Visit Preview Sep 5, 2022 at 1:30PM (UTC)
1 Ignored Deployment
Name Status Preview Updated
reakit ⬜️ Ignored (Inspect) Sep 5, 2022 at 1:30PM (UTC)

@diegohaz
Copy link
Member

diegohaz commented Aug 31, 2022

You can keep the old versions (and ariakit-react-utils with the same version as ariakit-utils). Just add it to https://github.com/ariakit/ariakit/blob/main/.changeset/pre.json

I published ariakit-react-utils to npm (looks like npm fails if the package doesn't exist in the registry).

@sarayourfriend
Copy link
Contributor Author

Let's see if I did it right 😅

@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 31, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit df707f0:

Sandbox Source
Ariakit Configuration

@sarayourfriend
Copy link
Contributor Author

Okay, looks like this could be ready to review 🙂 I'll update the PR description with suggestions for how to review this.

I was wondering if it'd be okay to add a readme.md to ariakit-utils with a note about not including any React imports in the package? Just wanting to make sure the reason for the split there is documented outside just this PR, if that's something you'd like, Diego.

diegohaz
diegohaz previously approved these changes Sep 5, 2022
Copy link
Member

@diegohaz diegohaz left a comment

Choose a reason for hiding this comment

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

Impressive work, Sara! Thank you very much!

packages/ariakit-react-utils/CHANGELOG.md Outdated Show resolved Hide resolved
packages/ariakit-utils/CHANGELOG.md Outdated Show resolved Hide resolved
@diegohaz
Copy link
Member

diegohaz commented Sep 5, 2022

I was wondering if it'd be okay to add a readme.md to ariakit-utils with a note about not including any React imports in the package? Just wanting to make sure the reason for the split there is documented outside just this PR, if that's something you'd like, Diego.

I think we could add an entry to the advanced part of the contributing guide. I'm trying to centralize all the contributing-related stuff there. Maybe Writing utils or something.

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.

None yet

2 participants