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

Refactor/simplify fetch patterns #121

Closed
wants to merge 3 commits into from

Conversation

@paulfalgout
Copy link
Member

paulfalgout commented Nov 5, 2019

  • Add the Clubhouse Story ID: [ch21319]

OK.. This PR does 2 things:

Instance Fetch
I'm not thrilled with modifying the documented behavior of Bb's fetch however as you'll see it greatly reduces the complexity of using fetch in every instance we use it. Also on success/fail it is still returning the jqxhr data, but on success it's inserting the instance of the model or collection which in general is what we want. We may get to a place where we really need the jqxhr to be returned by the fetch and I think in that instance we can add an options.useXhr or something that would return that instead of the instance promise.

beforeStart accepts an array of promises
This one should likely remove our need for really ever using $.Deferred outside of the bootstrap service. See the effects here https://github.com/RoundingWell/care-ops-frontend/compare/refactor/simplify-fetch-patterns?expand=1#diff-d6e4c48d0be5ae617f4aed9e9f2dc7a2R10
Two things to note with this change.
When one thing is resolved it'll be onStart(options, instance, jqData, etc) when an array is used it'll be onStart(options, [instance, jqData, etc], [instance, jqData, etc], ...)
so

beforeStart() {
  return Radio.request('entities', 'fetch:foo:model', id);
},
onStart(options, foo)

and

beforeStart() {
  return [
    Radio.request('entities', 'fetch:foo:model', id),
    Radio.request('entities', 'fetch:bar:model', id),
  ];
},
onStart(options, [foo], [bar])

Also with promises.. $.whening something that is not a promise would resolve it's value immediately.. so now arrays will act funny. here's what I mean:

// Simple value
beforeStart() {
  return 1;
},
onStart(options, one)

// Array before this PR
beforeStart() {
  return [1,2,3];
},
onStart(options, array)

// Array after this PR
beforeStart() {
  return [1,2,3];
},
onStart(options, one, two, three)

we rarely if ever resolve a value.. and if we do it'd probably be a model/collection instance.. we'll probably never ever return an array of values.. but something to keep in mind.

Copy link
Member

luketlancaster left a comment

looks good. Really seems to clean up what's becoming more commonplace around the app, so 👍 from me

@cypress

This comment has been minimized.

Copy link

cypress bot commented Nov 5, 2019



Test summary

61 0 0 0


Run details

Project RoundingWell Care Ops Frontend
Status Passed
Commit f9e4706
Started Nov 5, 2019 6:00 PM
Ended Nov 5, 2019 6:01 PM
Duration 01:09 💡
OS Linux Debian - 9.6
Browser Electron 73

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@codecov

This comment has been minimized.

Copy link

codecov bot commented Nov 5, 2019

Codecov Report

Merging #121 into develop will increase coverage by 21.14%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##           develop     #121       +/-   ##
============================================
+ Coverage    77.12%   98.26%   +21.14%     
============================================
  Files          104      104               
  Lines         1862     1847       -15     
============================================
+ Hits          1436     1815      +379     
+ Misses         426       32      -394
Impacted Files Coverage Δ
src/js/apps/forms/form/form_app.js 100% <100%> (ø) ⬆️
src/js/entities-service/clinicians.js 100% <100%> (ø) ⬆️
src/js/base/model.js 100% <100%> (+27.27%) ⬆️
src/js/services/bootstrap.js 100% <100%> (ø) ⬆️
.../apps/patients/patient/dashboard/add-action_app.js 100% <100%> (+10.52%) ⬆️
src/js/base/collection.js 100% <100%> (ø) ⬆️
src/js/base/entity-service.js 100% <100%> (+4.16%) ⬆️
src/js/base/app.js 100% <100%> (ø) ⬆️
src/js/components/picklist/index.js 100% <0%> (+2.38%) ⬆️
src/js/utils/formatting.js 87.8% <0%> (+2.43%) ⬆️
... and 38 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3165c08...f9e4706. Read the comment docs.

paulfalgout added 3 commits Nov 5, 2019
This allows a simpler pattern
This greatly reduces the complexity of what will be a common use case.  However it means
```
beforeStart() {
  return [1,2,3];
},
onStart(options, one, two, three)
```
where as before it would have been `onStart(options, array)`
3.6.0 fixes the issues seen in 3.5

The test modification is because previously we had a lazy test that got away with some ids not linking up, but returning the instance makes this a bit more strict in our id alignment in testing.
@paulfalgout paulfalgout force-pushed the refactor/simplify-fetch-patterns branch from 7d94856 to f9e4706 Nov 5, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Nov 5, 2019

Codecov Report

Merging #121 into develop will decrease coverage by 6.51%.
The diff coverage is 94.11%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #121      +/-   ##
===========================================
- Coverage    98.28%   91.77%   -6.52%     
===========================================
  Files          104      104              
  Lines         1862     1847      -15     
===========================================
- Hits          1830     1695     -135     
- Misses          32      152     +120
Impacted Files Coverage Δ
src/js/apps/forms/form/form_app.js 5% <0%> (-95%) ⬇️
src/js/entities-service/clinicians.js 100% <100%> (ø) ⬆️
src/js/base/model.js 75% <100%> (-25%) ⬇️
src/js/services/bootstrap.js 100% <100%> (ø) ⬆️
.../apps/patients/patient/dashboard/add-action_app.js 100% <100%> (ø) ⬆️
src/js/base/collection.js 100% <100%> (ø) ⬆️
src/js/base/entity-service.js 100% <100%> (ø) ⬆️
src/js/base/app.js 100% <100%> (ø) ⬆️
src/js/apps/forms/forms-main_app.js 0% <0%> (-100%) ⬇️
src/js/views/forms/form/form_views.js 25% <0%> (-75%) ⬇️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3165c08...f9e4706. Read the comment docs.

@paulfalgout paulfalgout closed this Nov 5, 2019
@paulfalgout paulfalgout deleted the refactor/simplify-fetch-patterns branch Nov 5, 2019
@paulfalgout

This comment has been minimized.

Copy link
Member Author

paulfalgout commented Nov 5, 2019

For future viewers this did get merged. github was having a sad and so the merge for whatever reason wasn't reflected on the PR

paulfalgout added a commit that referenced this pull request Nov 7, 2019
…terns

Refactor/simplify fetch patterns
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.