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

Allowing for asynchronous actions in afterSet and delegating loadData responsibility to setFilter #22

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

hardingalexh
Copy link
Contributor

This pull request is in response to #20.

In getting a subset of features implemented in the MBEE application, we found that beforeSet, afterSet, and loadData were all simultaneously firing - asynchronous behavior such as fetch requests were not properly awaited.

Context

In Harness applications, it is generally expected that setting a filter runs the loadData function. This is actually not a convention in harness-vue, but a convention in harness-vue-bootstrap, which are typically used in combination. All form control components in harness-vue-bootstrap use the same boundValue (link) computed function as a getter/setter in a two-way bind using Vue's v-model. This boundValue implementation checks for a synchronous prop, a harness-vue-bootstrap convention, and runs loadData.

harness-vue provides beforeSet and afterSet hooks to all filters, which use pinia's action susbcriptions to run before/after the setFilter action for a specific filter fires.

Problem

The initial discovery of an issue was for the MBEE project. In this project, the filter options lived on the server side, so there were frequent fetch requests being triggered in beforeSet and afterSet hooks. For example, setting filter A might trigger a fetch request to the server which is used to set the options and new default value for filter B.

What we noticed was that none of these fetch requests were properly awaited - beforeSet, loadData and afterSet were seemingly triggered simultaneously and resolved independently.

The problem here was twofold - first, none of the harness-vue action subscription implementation was asynchronous. Second, the boundValue computed setter in harness-vue-bootstrap runs setFilter and loadData independently. Because of the design of the pinia subscriptions, there is no way to have the setFilter function wait for any after effects to resolve as part of its own promise resolution.

Solution

The solution to the first problem was easy(ish) - we revised the implementation of setFilter and loadData to be asynchronous, as well as the subscription implementation. What we found in doing so is that pinia's action subscriptions handle after effects asynchronously, but not before effects. This seems to be intended, and means that in our implementation, we can expect that after effects are properly asynchronous, but before effects are not - which is fine and is a limitation we can document.

However, given how these things resolve independently, there was not a way to have harness-vue-bootstrap wait for the afterSet's promise resolution before running loadData.

Our solution is instead to bring this behavior into harness-vue. In this implementation we have changed the API so that synchronous is no longer a prop defined by harness-vue-bootstrap, but is instead now an attribute on harness-vue filters called triggerLoadData that defaults to false. This allows us to move the loadData call into the action subscription, so that we can properly expect a linear progression from setFilter -> afterSet -> loadData.

In doing so, we realized that sometimes we call setFilter in loadData or afterSet functions, but don't necessarily want to spawn a duplicate loadData call. To alleviate this, we added an optional parameter to setFilter (defaulting to true) that tells harness-vue whether or not to call loadData after the resolution of setFilter and afterSet.

To summarize the proposed API changes:

  • loadData is now a direct result of setFilter
  • setFilter can now be called as setFilter(key, payload, triggerLoadData = true)
  • filters can be given an attribute of triggerLoadData: false if you would never like them to trigger loadData as a side effect

I'm pausing here to check with the team in a call on Monday as to what ramifications this may have on existing codebases. I have a few thoughts that I'll capture there - specifically around the triggerLoadData parameter of setFilter.

@hardingalexh
Copy link
Contributor Author

Testing versions are available to try as @rtidatascience/[email protected] and @rtidatascience/[email protected].

You can update to both by running npm install @rtidatascience/[email protected] @rtidatascience/[email protected].

Expected Breaking Changes

None. Please make sure that when you trigger actions that would normally run setFilter (ie: hvb input changes, your own afterSet functions), you don't see loadData running multiple times.

Expected New Features

You should now be able to run setFilter(<filter>, <payload>, true) if you would like to have loadData run after setFilter.

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