Skip to content

Conversation

@jsnajdr
Copy link
Member

@jsnajdr jsnajdr commented Jul 11, 2024

I noticed that there is a duplicate version of Ariakit packages installed in packages/dataviews/node_modules so in this PR I'm deduping them. There is a good enough version (0.3.2) in the root node_modules, shared with packages/components.

@jsnajdr jsnajdr added the [Type] Code Quality Issues or PRs that relate to code quality label Jul 11, 2024
@jsnajdr jsnajdr requested review from ciampo and youknowriad July 11, 2024 13:23
@jsnajdr jsnajdr self-assigned this Jul 11, 2024
@github-actions
Copy link

github-actions bot commented Jul 11, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: jsnajdr <[email protected]>
Co-authored-by: youknowriad <[email protected]>
Co-authored-by: ciampo <[email protected]>
Co-authored-by: Mamaduka <[email protected]>
Co-authored-by: DaniGuardiola <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@jsnajdr
Copy link
Member Author

jsnajdr commented Jul 11, 2024

Haha! So the dataviews package declares a dependency on @ariakit/[email protected] but in reality it secretly depends on 0.4.1! Because it uses <Combobox autoSelect="always"/> and the always value is supported only since 0.4.1:

https://github.com/ariakit/ariakit/blob/main/packages/ariakit-react-core/CHANGELOG.md#new-autoselect-mode

That's why the build fails with type errors. We should probably upgrade Ariakit in the entire repo. @ciampo is that easy? 😉

@youknowriad
Copy link
Contributor

Interesting, the other thing I noticed that we import all of Ariakit import *. Is that the right approach? I feel like we should just import what we need otherwise tree shaking won't work (if that's supported by ariakit)

@ciampo
Copy link
Contributor

ciampo commented Jul 11, 2024

@jsnajdr I was literally typing a message very similar to yours pointing to https://ariakit.org/changelog#new-autoselect-mode

We should probably upgrade Ariakit in the entire repo. @ciampo is that easy?

@DaniGuardiola is already on it #62947 — I agree that it's better to update ariakit to the latest version and then dedupe

We should definitely aim at updating ariakit frequently to avoid having a lot of changes to test.

we import all of Ariakit import *. Is that the right approach? I feel like we should just import what we need otherwise tree shaking won't work (if that's supported by ariakit)

That's the suggested approach from ariakit, and I believe it should work with tree shaking — but I'll let @diegohaz reply here.

@jsnajdr
Copy link
Member Author

jsnajdr commented Jul 11, 2024

we import all of Ariakit import *. Is that the right approach?

That's a matter of code conventions and taste, webpack tree shaking is not affected. Webpack is able to figure out which imports are actually used by the module and ignores the rest.

@ciampo ciampo mentioned this pull request Jul 11, 2024
12 tasks
@Mamaduka
Copy link
Member

Mamaduka commented Aug 3, 2024

Are there any blockers for this PR?

@DaniGuardiola
Copy link
Member

DaniGuardiola commented Aug 3, 2024

@Mamaduka done in #64066, this PR should be closed.

@Mamaduka
Copy link
Member

Mamaduka commented Aug 3, 2024

Thanks, @DaniGuardiola!

@Mamaduka Mamaduka closed this Aug 3, 2024
@Mamaduka Mamaduka deleted the dedupe/dataviews-ariakit branch August 3, 2024 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Type] Code Quality Issues or PRs that relate to code quality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants