web4bio / webgen

WebGen Vertically Integrated Project
https://web4bio.github.io/webgen/main/html/
11 stars 26 forks source link

ENH: add helper function to fetch from firebrowse API #329

Closed kaczmarj closed 3 years ago

kaczmarj commented 3 years ago

This PR proposes adding a helper function fetchFromFireBrowse. This is a common operation in our codebase, and putting the meat of the logic in one function makes sense.

inspiration for this PR comes from https://github.com/web4bio/webgen/pull/322#issuecomment-944838293

please don't merge this until #322 is merged. once that is done, i will update the one remaining function that uses the older fetching methods.

kaczmarj commented 3 years ago

this is ready for review.

enemeth19 commented 3 years ago

This looks okay. I would maybe add a slightly modified version of

const minimal_json = { mRNASeq: [] };
    if (!response.ok) {
        console.log("Fetching mRNASeq data was unsuccessful.")
        return minimal_json
    }
    const json = await response.json();
    // We expect at least an mRNASeq key, so if this json object is empty, there's
    // a problem.
    if (!json) {
        console.log("mRNASeq is empty, returning an object with empty mRNASeq")
        return minimal_json
    }
    return json

to the helper function so that we can check for errors in the same way across all fetches from FireBrowse. What do you think?

kaczmarj commented 3 years ago

i agree that we should handle errors in the fetch helper. but what do you think we should do about the minimal return object if the fetched json is empty? it would be different for every function.

enemeth19 commented 3 years ago

Hm.. For const minimal_json = { mRNASeq: [] };, we could use regex to have the array label be whatever comes after the last forward slash in the endpoint argument. The same could be done for the console.log message

kaczmarj commented 3 years ago

good idea. i was wondering whether that would work on an endpoint like /Analyses/Mutation/MAF but just tested it and the returned object has a MAF key only. see https://firebrowse.herokuapp.com/?http://firebrowse.org/api/v1/Analyses/Mutation/MAF?format=json&tool=MutSig2CV&page_size=250&cohort=COAD

i'll add that to the fetch helper.

enemeth19 commented 3 years ago

Awesome, thanks so much!