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

Don't use Pinia for requestData even in development #1050

Merged
merged 5 commits into from
Nov 19, 2024
Merged

Conversation

matthew-white
Copy link
Member

@matthew-white matthew-white commented Nov 8, 2024

This PR removes Pinia from requestData entirely. requestData was designed to use Pinia in development (never production). However, we haven't been using Pinia for a while due to #787. That means that this PR doesn't have a huge practical effect. However, it simplifies requestData and opens the door to some extensions to it. For example, for #1044, I want to be able to create an arbitrary number of requestData resources, yet I'm not sure that that's something that would play well with our Pinia setup.

I think it's helpful to remember why we're using Pinia in the first place. We don't need Pinia for reactivity, because we get that out of the box with Vue 3. Instead, the main value-add of Pinia is logging. Specifically, Pinia provides a nice interface within Vue DevTools to see what changes have been made to requestData and which components specifically made those changes.

When I created requestData, it felt important for it to offer robust logging. requestData was a replacement for Vuex, which had its own mechanism for logging changes. At first, I started out writing my own logging mechanism. However, once I realized that Pinia integrated with Vue DevTools, using Pinia seemed like a reasonably straightforward and feature-rich way to incorporate logging into requestData.

It's been a while since then, and it's now clear that Pinia's logging hasn't proven to be especially useful. I expected there to be more issues with requestData, but thankfully, they just haven't come up that often. Given the infrequency of issues, it's uncommon that we need to troubleshoot requestData. When we do, console.log() is usually enough to do the job.

So it doesn't seem like Pinia is particularly useful. Yet at the same time, it also comes with costs:

  • The additional maintenance around one more Vue library. #787 is an example of that.
  • The patch() pattern that's removed in this PR exists only for the sake of logging. It helps to group together multiple changes as a single event within Pinia logging. However, it requires explanation, especially for newcomers to the codebase.
  • Pinia requires us to jump through some hoops in order for it to track which component is using which resource. The result is that the core logic underlying requestData is harder to follow.
  • Local resources aren't strictly isolated from one other, running counter to the whole concept of local resources. Two local resources with the same name would end up using the same Pinia store, which doesn't work. We were able to work around this issue, because it was never the case that different components on the same page would create local resources with the same name. However, it required some subtle management of requestData resources. The AsyncRoute component is an example of that that you can see in this PR.

What has been done to verify that this works as intended?

Tests continue to pass.

Before submitting this PR, please make sure you have:

  • run npm run test and npm run lint and confirmed all checks still pass OR confirm CircleCI build passes
  • verified that any code or assets from external sources are properly credited in comments or that everything is internally sourced

@matthew-white matthew-white requested a review from ktuite November 8, 2024 21:15
Copy link
Member Author

@matthew-white matthew-white left a comment

Choose a reason for hiding this comment

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

Notes from interactive code review

src/components/async-route.vue Show resolved Hide resolved
@matthew-white matthew-white merged commit 45d9c20 into master Nov 19, 2024
1 check passed
@matthew-white matthew-white deleted the remove-pinia branch November 19, 2024 17:18
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.

2 participants