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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add '@vue/composition-api' plugin; refactor SelectAddressForm with setup(); implement #7345 #8118

Merged
merged 14 commits into from Jun 7, 2021

Conversation

jonboiser
Copy link
Contributor

@jonboiser jonboiser commented May 26, 2021

Summary

  1. Adds @vue/composition-api@1.0.0-rc9 (seems fine to me and the final should be out sometime in Q3), so we can finally use it and take advantage of its benefits.
  2. Adds vueuse, which is a large library of utility functions that work with the Vue composition api.
  3. Factors out all the network location code out of SelectAddressForm into two "usables": useDynamicAddresses and useSavedAdddresses, and moving the object exposed from the usables into SelectAddressForm's setup function. Why is this cool? Because now all the code related to the two types of addresses can live in their own module, instead of being interlaced in lots of different parts of the Vue component. There are opportunities for cleaning up the API from the usables; I just wanted to do a direct 1-to-1 translation from Vue option -> usable without too much extra refactoring. This is also an opportunity to increase the test coverage of this code without having to worry about the extra complexities of unit testing the Component they live in.
  4. Fixes a bug where if you add an address, then select a different address right after, then the radio button for the added address will be selected after the polling for the dynamic addresses.
  5. Implements Make the last used address/device pre-selected in following imports聽#7345 by using https://vueuse.org/core/usestorage/#usage, which lets you create a reactive proxy to a local storage value.
  6. turns on the ESLint 'no-shadow' rule (which can cause a lot of confusion when you don't see it 馃槧 ) Had to revert it bc there's a lot of this in the codebase (needs followup issue)

Video of unreported bug that was fixed:

CleanShot 2021-05-26 at 11 10 10

References

Fixes #7345, and hopefully makes it easier to "fix" #7785 (which seems to be an issue with how static addresses are being filtered within useSavedAddresses)

Reviewer guidance

  1. Does the UI work
  2. Review the code carefully. Within setup and inside the usable's implementation, notice how I need to access ref.value, but outside of these functions (e.g. inside the template or options.computed, I don't need to do that (since those variables are now reactive proxies). Are there any places in the code where I overlook those caveats?

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md
    Use Vue Composition API plugin

Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

Do you think there are any notable changes to how we'd approach testing the components? It might help with team adoption in general to see how testing is affected by this change even if it's just a matter of composition-friendly code being bundled into modules with smaller and more specific scope being naturally easier to test.

@jonboiser
Copy link
Contributor Author

Do you think there are any notable changes to how we'd approach testing the components? It might help with team adoption in general to see how testing is affected by this change even if it's just a matter of composition-friendly code being bundled into modules with smaller and more specific scope being naturally easier to test.

I don't think anything really needs to change in terms of testing the component. You could test the "usables" and the components separately (and mock usable stuff in the context of the component test), but that would be a weaker test, since the two specs could get out of sync.

But pragmatically, what I would do is write the "usable" first, give it a stable API, and give it high test coverage. Then integrate it into a component, but only cover a subset of the component specific code, especially if mocking the XHR stuff is a pain. Then together, you have pretty good test coverage and the gaps can be filled in over time.

Copy link
Contributor

@micahscopes micahscopes left a comment

Choose a reason for hiding this comment

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

I think this is great. I also tried it and it seemed to work. I have a few points of feedback:

  • I've longed for this kind of thing
    • Now that it's here, I realize how much of a shift it is
    • It'd be great to do almost a mini workshop hack session for people on the team to understand how/when to use this stuff and how to think about it, and also how to understand the "use" metaphor
  • I had a hard time getting this branch building at first, and needed to try all kinds of things like make clean, switching to develop and back and deleting node_modules. I don't think it has anything to do with this PR, but just a heads up that I ran into these issues.

import { get, set, useIntervalFn } from '@vueuse/core';
import { fetchDynamicAddresses } from './api';

const Stages = Object.freeze({
Copy link
Contributor

Choose a reason for hiding this comment

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

nice! you learn something new every day, and today I learned about this

@jonboiser jonboiser merged commit c3a6c46 into learningequality:develop Jun 7, 2021
@jonboiser jonboiser deleted the save-net-addresses branch June 7, 2021 17:45
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.

Make the last used address/device pre-selected in following imports
3 participants