57 - Clean up the no longer used resources and functions

[nozomione]

Issue Number

Closing #57

Purpose/Implementation Notes

Cleaned up the no longer used resources and helper functions and its corresponding test, and adjusted the codbase.

Types of changes

• New feature (non-breaking change which adds functionality)

Functional tests

List out the functional tests you've completed to verify your changes work locally.

Checklist

• Lint and unit tests pass locally with my changes

[davidsmejia]

Could you help me understand why this PR seems to depart from the docs? It looks like the "listing" response has been moved into the get functionality and away from the filtering functionality here. My understanding is that get should be used when you want to fetch a single resource from the API and the filter should be used when you want to receive multiple resources from the API.

Per the docs:
Our API supports the following actions:

Action	Description
get	sends a GET request and returns a single object
filter	sends a GET resuest with a query string and returns a list of objects(maybe paginated)

[nozomione]

Could you help me understand why this PR seems to depart from the docs? It looks like the "listing" response has been moved into the get functionality and away from the filtering functionality here. My understanding is that get should be used when you want to fetch a single resource from the API and the filter should be used when you want to receive multiple resources from the API.

Per the docs: Our API supports the following actions:

Action	Description
get	sends a GET request and returns a single object
filter	sends a GET resuest with a query string and returns a list of objects(maybe paginated)

The filter action is not necessary for fetching some resources (02cc53f), since we can use our get action (defined in getActions) to do so (whether it's paginated or not).

Based on our current implementation of the get action, it can do the following:

• get() -> return a list of resources
• get(id) -> return a specific resource with the given id(e.g, name, accession code)

And the get action also takes the optional id and query parameters, and that query can be the query string for returning the paginated resources (e.g., a limit and offset for the pagination - the common query string we typically pass to fetch the resources from API).

For this reason, for instance, I simpified the organism resource and its test by removing the filter action, since we can fetch a list of organisms or a specific organism with the given organism name using the get action.

For the search resource, we still use the filter action for the advanced filtering (but we can also use the get action).

Please let me know your thoughts on this @davidsmejia 💬

[davidsmejia]

The filter action is not necessary for fetching some resources (02cc53f), since we can use our get action (defined in getActions) to do so (whether it's paginated or not).

Based on our current implementation of the get action, it can do the following:

* `get() -> return a list of resources`

* `get(id) -> return a specific resource with the given id(e.g, name, accession code)`

And the get action also takes the optional id and query parameters, and that query can be the query string for returning the paginated resources (e.g., a limit and offset for the pagination - the common query string we typically pass to fetch the resources from API).

For this reason, for instance, I simpified the organism resource and its test by removing the filter action, since we can fetch a list of organisms or a specific organism with the given organism name using the get action.

For the search resource, we still use the filter action for the advanced filtering (but we can also use the get action).

Please let me know your thoughts on this @davidsmejia 💬

Yeah I see what you mean that it is currently possible to do that. The get method should never return anything besides an individual resource because it is a departure from how APIs and ORMs work in general and that is what we emulating here. The idea is that you can "get" an item and "filter" a list of items. I would rather see the default behavior be throwing an error if an ID is not supplied for the "get" request because there is no way to figure out what they want to "get".

I am open to considering renaming get/filter to something else but the distinction between these functionalities is important to preserve.

[nozomione]

Yeah I see what you mean that it is currently possible to do that. The get method should never return anything besides an individual resource because it is a departure from how APIs and ORMs work in general and that is what we emulating here. The idea is that you can "get" an item and "filter" a list of items. I would rather see the default behavior be throwing an error if an ID is not supplied for the "get" request because there is no way to figure out what they want to "get".

Given that this is a JS API client and that the GET method in web development refers to 'fetching the resource(s)' from the server (e.g, resource: HTTP GET method), using the get action to fetch the resource(s) typically makes sense here (also the term get might be more friendly to front-end developers compared to using filter to fetch the resource(s)).

Perhaps, we can use the term get for fetching a list of resources and use the term filter for fetching a specific resource (e.g., filter a list of resources with a specified id) 🤔 💭 ?

I am open to considering renaming get/filter to something else but the distinction between these functionalities is important to preserve.

👍

I am open to different approaches/ideas as well and to improve this API client 💡 and any recommended resources are welcome (I found an article on Python API and ORMs (with Django) but was too broad)!

[davidsmejia]

Given that this is a JS API client and that the GET method in web development refers to 'fetching the resource(s)' from the server (e.g, resource: HTTP GET method), using the get action to fetch the resource(s) typically makes sense here (also the term get might be more friendly to front-end developers compared to using filter to fetch the resource(s)).

Perhaps, we can use the term get for fetching a list of resources and use the term filter for fetching a specific resource (e.g., filter a list of resources with a specified id) 🤔 💭 ?

I am open to considering renaming get/filter to something else but the distinction between these functionalities is important to preserve.

👍

I am open to different approaches/ideas as well and to improve this API client 💡 and any recommended resources are welcome (I found an article on Python API and ORMs (with Django) but was too broad)!

This is not GET this is "get". I understand that looks weird but take a look at the docs. These functions are named after things you do to get / modify a resource they are not named after the underlying HTTP verb.

This is just convention, but the goal of creating something like refinebio-js is that people are not concerned with the underlying networking requests and can think of things more directly as objects / collections of objects.

Here are some examples of what I would expect to have a function named for what it does.

• create / new
• get / fetch / find
• filter / query / list / search
• update / save

Here is the section of the docs that shows its not a 1:1 correspondence of HTTP verb and function name.

Action	Description
create	sends a POST request and returns a new instance
get	sends a GET request and returns a single object
filter	sends a GET resuest with a query string and returns a list of objects(maybe paginated)
update	sends a PUT request and returns a single object

[nozomione]

This is not GET this is "get".

The term get action in the front-end web development generally refers to 'fetch' and I referenced the HTTP request GET method (via the REST API net as an example reference as to why helper methods for fetching the server resource(s) are typically named get<ResourceName> or fetch<ResourceName> in the front-end codebase).

Action	Description
create	sends a POST request and returns a new instance
get	sends a GET request and returns a single object
filter	sends a GET resuest with a query string and returns a list of objects(maybe paginated)
update	sends a PUT request and returns a single object

In the refinebio-js's README, we initially defined the filter action as above (also we defined it as "filter" takes a filter object and coverts it to a query parameter in the doc), but after working with the refine.bio backend API to create the refinebio-web UI, I realized that there is a potential for optimizing this refinebio-js codebase, such as to clean up the unrequited helpers and to use the get action to fetch resource(s) which may be more front-end friendly (it might be nice), since the main target audience for this tool is front-end developers who use it to make web apps for refiene.bio (originally discussed).

Here are some examples of what I would expect to have a function named for what it does.

• create / new
• get / fetch / find
• filter / query / list / search
• update / save

Let's chat on the above actions and how we can improve them.

In the meantime, let me know if you want me to revise the commit 02cc53f back for now, thank you @davidsmejia !

[davidsmejia]

Ok so sounds like its just the name that is tripping you up. I am sure we can find a reasonable alternative. Please do roll back that commit for now and we can discuss in another issue.

Just a reminder this is not just for the FE but is just a javascript implementation of an API client that is modeled after a variation of CRUD. CRUDL - which is the same thing but with List as a separate action. To be fair I always call "CRUDL" "CRUD" because that listing action is sort of assumed. The benefit is that when calling what we currently call filter (the list action) on success / isOK we are guaranteed a list of resources of arbitrary length and our currently called get (the read action) on success / IsOK will guarantee a single resource in the response.

The idea is that one would use the read action on the detail page, so a FE example would be when you present a single experiment like experiments/[accession] you would load the data for that via api.experiments.get(accessionFromQueryParam) and to populate the samples table you would do api.samples.filter(someFilteringOptionsOrEmpty). Removing the possibility that you could get a single resource for samples or multiple experiments in those examples removes the need for the end user to have to inspect the response to check if its wrapping in an array after using the get method that provides both single and multiple responses.

Ping me when that is rolled back and I will review then, thank you!

[nozomione]

Ok so sounds like its just the name that is tripping you up. I am sure we can find a reasonable alternative. Please do roll back that commit for now and we can discuss in another issue.

Not only the naming but also the usability, since we plan to use this client API for developing web UI.

The key responsibilities of our refinebio web app are currently the following;

• to fetch and render the server-side resource(s) (a BE API response) in UI
  • by default, we have a size limit on the number of resources that can be fetched from the server
  • as needed, we re-fetch the resources with the user-requested filter
• to fetch and render the user-created dataset (by id) and let them download it (or one-off experiment by accessionCode)
• to display an error in UI if any (*we'll improve the FE error handling implementation in a new issue)

For the samples table, we currently do the following operations:

• to fetch and render the table data with the specified experiment (by accessionCode) with the size limit (10, 20, 50)
  • as needed, we re-fetch the sample(s) with the user-requested filter
• to add/remove a sample to/from the user's dataset
  • e.g., via addSample(id)/removeSample(id) with the update action
• to display an error in UI if any

Removing the possibility that you could get a single resource for samples or multiple experiments in those examples removes the need for the end user to have to inspect the response to check if its wrapping in an array after using the get method that provides both single and multiple responses.

Currently, when the FE requests the samples (for samples table) via the BE API, the BE returns a single server response which includes thecount and the results array which includes the all available samples. A user (FE developer) may obtain the length of samples by the count or checking the length of the results arary:

// no result 
{
  "count": 0,
   ... 
  "results: [] 
}

// a single sample
{
  "count": 1,
   ... 
  "results:  [{"id":..."}] 
}
// multiple samples
{
   "count": 3
   ... 
  "results:  [{"id":..."}, {"id":..."}, {"id":..."} ] ,
}

Technically, whether we used the filter (CRUDList) or the get action (CReadUDL), we would still perform the same check on the FE in order to render the BE response in UI (e.g., to make sure the BE response is not empty).

Also if we decide to introduce CRUDList in refinebio-web, it requires codebase adjustments which differs from the standard REST API web UI development (in terms of naming and UI rendering in FE etc) and we'll need to discuss about that as well.

Perhaps we should re-target the main audience of the refinebio-js tool. I reverted back the previous commit (f16c224), thank you @davidsmejia !

[davidsmejia]

Please revert the changes on everything in the resources folder. The removal of filtering from jobs and stats should not be in this PR either. I expect this PR to only contain changes to the helpers, specifically removing code that does not get used.

[nozomione]

Please revert the changes on everything in the resources folder. The removal of filtering from jobs and stats should not be in this PR either.

@davidsmejia So we should open a separate PR for the resource folder for the updates (I only ask because on 1:1, we already chatted about this issue that includes the resource folder)?

[jaclyn-taroni]

@nozomione, please update this PR in response to this comment:

Please revert the changes on everything in the resources folder. The removal of filtering from jobs and stats should not be in this PR either. I expect this PR to only contain changes to the helpers, specifically removing code that does not get used.

I'd break the resources part of #57 out into a new ticket for @davidsmejia's review. Thank you.

[nozomione]

@nozomione, please update this PR in response to this comment...I'd break the resources part of #57 out into a new ticket

@jaclyn-taroni completed, thank you!

@davidsmejia I revised back the updates for the resources. Regarding the job and stats, I'll hold off the adjustments for now since we'll keep the term filter.

Although I reverted back 287e32b, please let me know if it's okay to include the auto-generated yarn.lock file in this PR. Since I had to run a package install command after cloning a fresh copy of this repo on my computer, it auto-upgraded the minor and bug releases which are backward compatible. Thank you!

[davidsmejia]

Looking good, just had a couple concerns

[davidsmejia]

Suggested change

	newObj[key] = isObject(source[key]) ? deepCopy(source[key]) : source[key]
	newObj[key] = isObject(source[key]) || isArray(source[key]) ? deepCopy(source[key]) : source[key]

It looks to me that if a value is an array we would return the reference. We should probably create a copy here.

I think another approach to this function would to have a base state of an atomic type (not object or array), and then recursively handle objects and arrays.

[davidsmejia]

This test could be improved by adding another array inside of c and then testing if their reference is the same.

expect(output.c[output.c.length - 1]).not.toBe(input.c[input.c.length - 1])

[davidsmejia]

This signature no longer matches how it is used in src/utils/makeRequest