Draft: composition and defaults

[patrickarlt]

@tomwayson @dbouwman @jgravois I've been wanted to spend some in REST JS land so I thought I would take a crack at 2 related things:

1. Functional helpers like withUrl() and withOptions() from facilitate composition of our low level functional API to reduce end user friction #339.
2. Allow setting of global default options for all requests like authentication.

All of this works pretty well. Here is a summary of the changes:

1. break out all helpers that were living in request.ts into separate files.
2. Allow the method signature of request to accept:
   1. request(url)
   2. request(url, options)
   3. request(options) - with a url in the options, this is new and needed for withUrl() when withUrl() is used with request
3. requestOptions including params and headers are now merged with a globally exposted constant DEFAULT_ARCGIS_REQUEST_OPTIONS which can accept any IRequestOptions and use them as the default options for all requests.
4. We needed some newer typescript features so I moved us back to the offical rollup-plugin-typescript which doesn't bundle its own TS version.

This PR is still a draft and I need do a few things:

1. Write docs for DEFAULT_ARCGIS_REQUEST_OPTIONS, withUrl() and withOptions() as well as a guide topics for them
2. Write a new guide topic on how we structure our method signatures
3. Get code coverage back to 100%, there are a lot of new branches in this code to test
4. Use this in at least 1 demo
5. add withSession if we decide to add it
6. Fix stuff depending on our discussions

There are also still a few things to discuss:

1. I made the method signatures withUrl(fn, url) and withOptions(fn, options) since I thought it looked better and it makes the typing a little cleaner. I think this differs from most FP patterns which would normally write these as withUrl(url, fn) and withOptions(options, fn). I'm fine with flipping these.

2. Currently request does not work with withUrl(). This can be fixed but breaks other code and tests. You can see the commitment out fix here. This problem exists because request has a different method signature then every other method where it accepts the url as the first param rather then in the options.

   There are also issues with withUrl(request, "the url") and TypeScript. I cannot figure out how to properly make TypeScript accept that requestWithUrl from var requestWithUrl = withUrl(request, "the url") can be called like requestWithUrl(). TypeScript INSISTS on forcing requestWithUrl("")

   There are 3 options:

   1. Uncomment the fix this makes request prefer the urlin options over the actual URL param. This will break a bunch of tests for things like getAttachments which just format a url and pass as the first param for request. The fix for this is REALLY easy though

      export function getAttachments(
        requestOptions: IGetAttachmentsOptions
      ): Promise<IGetAttachmentsResponse> {
        // pass through
        return request(
          `${cleanUrl(requestOptions.url)}/${requestOptions.featureId}/attachments`,
          requestOptions
        );
      }
      
      // should become:
      
      export function getAttachments(
        requestOptions: IGetAttachmentsOptions
      ): Promise<IGetAttachmentsResponse> {
        // reformat URL with IGetAttachmentsOptions and pass through
        requestOptions.url = `${cleanUrl(requestOptions.url)}/${requestOptions.featureId}/attachments`;
        return request(requestOptions);
      }

   2. Accept that request cannot be properly typed with withURL() and document that fact for TypeScript users. JS users can still do this to their hearts content.
   3. Make a breaking change where request ONLY accepts options with a url in the key. Since most usage of the library is NOT request most of this can be an internal change.

3. Where should these helpers live? If we decide that withUrl() shouldn't work with request should it really live in the arcgis-rest-request package? Or should both of these live in arcgis-rest-request BECAUSE arcgis-rest-request is a dependency for every other package?

4. Do we still want to add withSession to arcgis-rest-authentication?

[jgravois]

gut reactions:

I made the method signatures withUrl(fn, url) and withOptions(fn, options) ... I'm fine with flipping these.

i don't feel strongly, but i'd prefer them flipped.

Currently request does not work with withUrl()

i'm actually fine with advertising that the new helper methods can't be used by TypeScript developers to wrap request.

Make a breaking change where request ONLY accepts options with a url in the key. Since most usage of the library is NOT request most of this can be an internal change.

i don't think allowing folks to use request in new helpers is a big enough pay off to make the breaking change you suggest worth it. i'm thinking back to the confusion we caused when we introduced a similar change in Esri Leaflet 1.0.0.

you're right that most of our API is already wrappers, but i've seen lots of folks just using request too.

Where should these helpers live?

@esri/arcgis-rest-common

Do we still want to add withSession to arcgis-rest-authentication?

i'm fine with kicking the can down the road on this one.

[patrickarlt]

@jgravois allowing syntax like:

request({
   url: "https://"
})

isn't actually a breaking change. What would be a breaking change (at least internally) in that in order to make request prefer options.url over url breaks ~17 tests. I can change the implimentation to match the new spec so users won't see a breaking changes.

The breaking change to request would come if we decode to remove support for request(url) and just allow request(options)

[jgravois]

I'm pickin up what you're puttin down.

if it's only 17 tests, I can help rejigger the relevant methods. I just don't want to deprecate request(url, options) entirely.

[tomwayson]

I really like the idea of being able to set defaults.

However, I've run into problems using module scoped variables for things like user sessions in server-rendered apps. When there's only one instance of the server running that is handling multiple requests, the request module will be shared across those requests. So if request A sets DEFAULT_ARCGIS_REQUEST_OPTIONS.authentication for user A, and then an unauthenticated request B comes in it will get user A's auth appended whenever it calls request(). 😱

Module scoped variables work fine in the client, and they work fine on the server for things that can be and probably should be shared between requests, like the way you set headers in the test. So providing a way to set defaults would provide real value. Based on my poll of our DevSummit session where exactly 0 people raised their hands when I asked if anyone is doing server-rendering, I doubt this will be a problem for, well, hardly anyone. That said, I'd feel better about this if we could could come up w/ a way to only let users set defaults for things that make sense, or at least not set default authentication.

Also, rather than export the defaults directly, I'd rather export a fn like setDefaultRequestOptions(). That could be a place for us to help enforce whatever we decide above as well as prevent people from shooting themselves in the foot by setting them to 'foo' or 42.

[tomwayson]

I made the method signatures withUrl(fn, url) and withOptions(fn, options) since I thought it looked better and it makes the typing a little cleaner. I think this differs from most FP patterns

I think we should follow FP patterns

[tomwayson]

It's great that @patrickarlt has spiked through thinking how these related features will interact, but when it comes to actually merging PRs I think it would be good to decouple as much of this as possible.

It seems the sticky bit in all this is changing the signature of request(). Maybe I'm missing something, but couldn't we add the "with" helpers (at least withOptions()) and/or defaults w/o doing that?

It seems like @patrickarlt is coming to the conclusion that supporting either a url and options, or just options w/ options.url is not worth the complexity that it introduces. I am also wary of that complexity, but I have mixed feelings on whether or not we should leave the signature the way it is (and not put url in options in all cases), or always put url in options. One the one hand, I just don't feel like url belongs in options, and I'm not a fan of how we had to do that for most fn's in -feature-service. I only did that to conform w/ our API design guidelines, but I always felt that queryFeatures(url, options) would have been a better API, and to @dbouwman's point, it really feels like we're burying the lead here. In other words, part of me wants to resolve the above issues by getting rid of options.url everywhere. That seems impractical though, and my feelings on the API are just feelings, and I could go along w/ options.url being the one true way.

[patrickarlt]

the request module will be shared across those requests.

Based on my poll of our DevSummit session where exactly 0 people raised their hands when I asked if anyone is doing server-rendering, I doubt this will be a problem for, well, hardly anyone.

That said, I'd feel better about this if we could could come up w/ a way to only let users set defaults for things that make sense, or at least not set default authentication.

Setting up a default authentication is pretty much the only point of having default options for me. I'm fully aware that this will cause an issue server side which is any this is an opt in features and you can always override it by passing in an authentication option.

As a compromise could we export a function like:

export function setDefaultRequestOptions(options, hideWarnings = false) {
  if(options.authentication && !hideWarnings){
    warn("warn users about the dangers of globally setting authentication and link to doc.")
  }
  DEFAULT_REQUEST_OPTIONS = options;
}

It seems the sticky bit in all this is changing the signature of request(). Maybe I'm missing something, but couldn't we add the "with" helpers (at least withOptions()) and/or defaults w/o doing that?

@tomwayson right now inconsistant method signatures are causing a bit of pain with withURL. Consider:

request(url, options);

queryLayer(query, {
  url: "..."
})

It is impossible to make withURL work in both cases. The easy solution to this to also allow:

request({
  url: "..."
})

Which means we can consistently make the last options be IRequestOptions and we can always override the url property on it. Moving method signatures to:

queryLayer(url, {
  query: "..."
})

Just makes this problem even MORE difficult since url won't always be the first param since we don't need it for lots fo things like:

searchItems(searchQuery)

In this case the first param could also be a string.

You are absolutely right that we are burying the lead to url in the docs but that could be easily solved by sorting required options to the top as @jgravois suggested somewhere.

[patrickarlt]

I'll flip the method signatures since it seems we prefer:

withURL("url", fn);

withOptions({
  foo:"bar"
}, fn);

and I'll move these into common. Also this PR and #509 are now divorced.

[tomwayson]

Setting up a default authentication is pretty much the only point of having default options for me.

I know. Me too. I like your proposal for how we can warn users by default, but still let power users who (at least think they) know what they are doing set it and kill the warnings.

[tomwayson]

I'm fine w/ adding .url to IRequestOptions and having that be the only way we pass URLs around. I feel less strongly about that than I do about keeping -feature-service-admin as it's own package.

we are burying the lead to url in the docs but that could be easily solved by sorting required options to the top

I think @jgravois has tried and didn't find that to be easy. Also, in this particular case, url won't be required since most of the higher level functions like getItem() that call request() do not require a user to supply a URL b/c they build the URL internally.

This also raises a question of, how should those fns treat options.url if it's passed in? Ignore it? Let it be a way for the user to override the URL that they would otherwise build?

Or do we turn the existing IRequestOptions into something like IRequestOptionsBase to be used by all the fns that build URLs internally, and then add url (required) to IRequestOptions which would then be used by request() and what the fns that require URLs will extend... 🤔

No matter what we decide, I agree that this is more of a docs issue than anything, and that we will be able to find some solution even if it's a matter of always forcing url to go at the top whenever it's required.

[patrickarlt]

@jgravois This should also be ready for review. I've moved withOptions() to common and added the warning to a new setDefaultRequestOptions() method in addition to removing withUrl() and flipping the param order on withOptions().

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (v2.0.0@18b3967). Click here to learn what that means.
The diff coverage is 100%.

@@           Coverage Diff           @@
##             v2.0.0   #508   +/-   ##
=======================================
  Coverage          ?   100%           
=======================================
  Files             ?     94           
  Lines             ?   1352           
  Branches          ?    238           
=======================================
  Hits              ?   1352           
  Misses            ?      0           
  Partials          ?      0

Impacted Files	Coverage Δ
...ages/arcgis-rest-portal/src/util/get-portal-url.ts	100% <ø> (ø)
.../arcgis-rest-feature-service/test/mocks/service.ts	100% <ø> (ø)
.../arcgis-rest-portal/test/mocks/groups/responses.ts	100% <ø> (ø)
...cgis-rest-feature-service-admin/test/mocks/move.ts	100% <ø> (ø)
...ckages/arcgis-rest-portal/test/mocks/users/user.ts	100% <ø> (ø)
packages/arcgis-rest-portal/src/items/search.ts	100% <ø> (ø)
...rcgis-rest-request/src/utils/ArcGISRequestError.ts	100% <ø> (ø)
...eature-service-admin/test/mocks/layerDefinition.ts	100% <ø> (ø)
...s/arcgis-rest-feature-service/test/mocks/fields.ts	100% <ø> (ø)
...ckages/arcgis-rest-portal/test/mocks/items/item.ts	100% <ø> (ø)
... and 9 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 18b3967...a579715. Read the comment docs.

[patrickarlt]

@jgravois I also reverts all the changes to request and IRequestOptions so the signatures shouldn't change.

[jgravois]

nice work @patrickarlt!

thanks for taking the time to split out all the utilities in request into their own files while you were digging around in there.

this branch has been synced and the PR base is now set to v2.0.0.

disclaimer: withOptions() now lives inside of request.

[pfcstyle]

I'm sorry! This is a wrong commit. Please ignore it.