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

feat(core): add aliases to mitigate context issues #7172

Merged
merged 3 commits into from
Jul 19, 2024
Merged

Conversation

ricokahler
Copy link
Contributor

@ricokahler ricokahler commented Jul 16, 2024

Description

This pull request enhances the sanity React library's build tooling by adding comprehensive comments and JSDoc annotations to improve maintainability and understanding of the code. Specifically, it elaborates on package exports, conditions, and the getAliases function. Additionally, it completes the associated test file to ensure the getAliases function operates correctly in both monorepo and normal build scenarios.

Key changes:

  • Added detailed comments and JSDoc annotations to aliases.ts to explain the purpose and functionality of the code.
  • Completed the test file aliases.test.ts with test cases for normal builds and monorepo builds.
  • Mocked monorepo development aliases in the test to ensure accurate alias resolution.

Why these changes were introduced:

  • To prevent multiple context errors due to multiple instances of the library.
  • To improve code readability and maintainability.
  • To ensure comprehensive testing coverage for the alias resolution functionality.

What to review

  • Review the added comments and JSDoc annotations in aliases.ts for accuracy and clarity.
  • Review the test cases in aliases.test.ts to ensure they cover the expected scenarios and edge cases.
  • Ensure that the monorepo aliases are correctly mocked and resolved in the tests.

Testing

  • Added test cases for both normal builds and monorepo builds in aliases.test.ts.
  • Verified that the dynamic resolution logic in the tests mirrors the resolution logic used in getAliases.
  • Testing manually by packing and linking this in the admin studio.

Demo:

aliases.mp4

Notes for release

  • Adds aliases to internal vite config used by sanity build and sanity deploy to prevent missing React context issues.

@ricokahler ricokahler requested a review from a team as a code owner July 16, 2024 20:45
@ricokahler ricokahler requested review from rexxars and removed request for a team July 16, 2024 20:45
Copy link

vercel bot commented Jul 16, 2024

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

Name Status Preview Comments Updated (UTC)
page-building-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 19, 2024 4:07pm
performance-studio 🛑 Canceled (Inspect) 💬 Add feedback Jul 19, 2024 4:07pm
test-compiled-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 19, 2024 4:07pm
test-next-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 19, 2024 4:07pm
test-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 19, 2024 4:07pm
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
studio-workshop ⬜️ Ignored (Inspect) Visit Preview Jul 19, 2024 4:07pm

Copy link
Contributor

No changes to documentation

Copy link
Contributor

github-actions bot commented Jul 16, 2024

Component Testing Report Updated Jul 19, 2024 3:52 PM (UTC)

File Status Duration Passed Skipped Failed
comments/CommentInput.spec.tsx ✅ Passed (Inspect) 44s 15 0 0
formBuilder/ArrayInput.spec.tsx ✅ Passed (Inspect) 8s 3 0 0
formBuilder/inputs/PortableText/Annotations.spec.tsx ✅ Passed (Inspect) 31s 6 0 0
formBuilder/inputs/PortableText/copyPaste/CopyPaste.spec.tsx ✅ Passed (Inspect) 36s 11 7 0
formBuilder/inputs/PortableText/copyPaste/CopyPasteFields.spec.tsx ✅ Passed (Inspect) 0s 0 12 0
formBuilder/inputs/PortableText/Decorators.spec.tsx ✅ Passed (Inspect) 17s 6 0 0
formBuilder/inputs/PortableText/DisableFocusAndUnset.spec.tsx ✅ Passed (Inspect) 10s 3 0 0
formBuilder/inputs/PortableText/DragAndDrop.spec.tsx ✅ Passed (Inspect) 2m 33s 1 0 0
formBuilder/inputs/PortableText/FocusTracking.spec.tsx ✅ Passed (Inspect) 43s 15 0 0
formBuilder/inputs/PortableText/Input.spec.tsx ✅ Passed (Inspect) 1m 44s 21 0 0
formBuilder/inputs/PortableText/ObjectBlock.spec.tsx ✅ Passed (Inspect) 1m 13s 18 0 0
formBuilder/inputs/PortableText/PresenceCursors.spec.tsx ✅ Passed (Inspect) 8s 3 9 0
formBuilder/inputs/PortableText/RangeDecoration.spec.tsx ✅ Passed (Inspect) 25s 9 0 0
formBuilder/inputs/PortableText/Styles.spec.tsx ✅ Passed (Inspect) 18s 6 0 0
formBuilder/inputs/PortableText/Toolbar.spec.tsx ❌ Failed (Inspect) 1m 23s 20 0 1
formBuilder/tree-editing/TreeEditing.spec.tsx ✅ Passed (Inspect) 1m 47s 30 0 0
formBuilder/tree-editing/TreeEditingNestedObjects.spec.tsx ✅ Passed (Inspect) 19s 3 0 0

@rexxars
Copy link
Member

rexxars commented Jul 16, 2024

Tests seem to be failing, will review when they pass!

@cngonzalez
Copy link
Member

These changes generally make sense to me and I'm sure work smoothly, but I'd feel more comfortable with someone more familiar with our Vite config having a look as well 🙇

cngonzalez
cngonzalez previously approved these changes Jul 19, 2024
@cngonzalez cngonzalez added this pull request to the merge queue Jul 19, 2024
Merged via the queue into next with commit 776f16b Jul 19, 2024
24 of 30 checks passed
@cngonzalez cngonzalez deleted the sdx-1471 branch July 19, 2024 15:37
stipsan pushed a commit that referenced this pull request Jul 22, 2024
* feat(core): add aliases to mitigate context issues

* fix(core): use `find` `replacement` syntax for aliases

* docs: add TODO comment about improving test
stipsan pushed a commit that referenced this pull request Jul 22, 2024
* feat(core): add aliases to mitigate context issues

* fix(core): use `find` `replacement` syntax for aliases

* docs: add TODO comment about improving test
cngonzalez pushed a commit that referenced this pull request Aug 20, 2024
* feat(core): add aliases to mitigate context issues

* fix(core): use `find` `replacement` syntax for aliases

* docs: add TODO comment about improving test
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.

3 participants