Reduce peerDependencies

[tomwayson]

Currently just about every package has 2 peerDependencies: request and auth. In my experience peerDependencies are hard for people to grok/manage. Ideally we'd have it so that all packages only have a single peer dependency, which would be request.

First, I don't think any of these packages need to even declare a dependency (peer or otherwise) on arcgis-rest-auth (they never import from it):

• arcgis-rest-feature-layer
• arcgis-rest-geocoding
• arcgis-rest-routing

We should remove those dependencies.

Second, arcgis-rest-service-admin only imports IUserRequestOptions from auth. We could move that interface to either the request or types package and drop the dependency on auth.

Side note, it looks like the types package is under dependencies of all the other packages, but it probably should be under devDependencies, right?

OK, finally, only the arcgis-rest-portal package actually imports a concrete implementation from auth, UserSession, but it only uses it type declarations (e.g. export function getUserUrl(session: UserSession): string). We can't move the UserSession class to the types package. We could move the dependency under devDependencies, but it would be up to us to make sure we don't introduce any runtime uses (like new UserSession()). Seems a bit risky. Maybe we just leave it as under peerDependencies in this case?

To me the only downside of moving away from having auth as a peerDependency is that we'll have to rely on the docs (alone) to get across the message that "You need to install @esri/arcgis-rest-auth if you need to make authenticated requests."

[tomwayson]

FYI - the above proposal is, an alternative, and I think a better one, to the idea we discussed in a call today of merging auth into request and drop the auth package. The downside of consolidation is that auth (14.9kb minified)is actually bigger than request (13.6kb minified) and it'd be a shame to more than double the size for anyone that truly doesn't need auth.

[patrickarlt]

Side note, it looks like the types package is under dependencies of all the other packages, but it probably should be under devDependencies, right?

I think we put this there so the types are always there for Typescript users. It doesn't have any impact on final bundle sizes though.

The downside of consolidation is that auth (14.9kb minified)is actually bigger than request (13.6kb minified) and it'd be a shame to more than double the size for anyone that truly doesn't need auth.

I wonder how much of that could be reduced by switching to ES2017 as a compile target in #785. In any event tree shaking would take care of it if you didn't use auth. The main benefit of separate modules is that if you DID screw up tree shaking it wouldn't bloat too much but I don't think this is too bad.

[tomwayson]

I think we put this there so the types are always there for Typescript users. It doesn't have any impact on final bundle sizes though.

I was thinking that TS consumers of the library would have everything they need the .d.ts files under /dist/esm/, but yeah, the fn signatures in those definitions would still reference interfaces/types defined in @esri/arcgis-rest-types. Perhaps #789 would help w/ that.

[patrickarlt]

@tomwayson 2 updates to this:

1. NPM 7 now automatically installs peer dependencies. https://github.blog/2021-02-02-npm-7-is-now-generally-available/#peer-dependencies
2. RE drop support for Internet Explorer and Edge Legacy #785 switching to ES2017 cut @esri/arcgis-rest-request from 13.6kb > 7.07kb. Weirdly the UMD bundle didn't shrink when rollup compiles it because it still thinks it is necessary to compile our class declarations. I'm not really sure what to make of it but we DO get some bundle size gains here.

[patrickarlt]

I was reading the output wrong gains from targeting ES2017 are not huge. 0.5kb for request and ~1kb for auth.

Package	ES5 (3.0)	ES2017 (4.0)
arcgis-rest-request
arcgis-rest-auth

[tomwayson]

@patrickarlt

I think we should still do this and get it into the 4.0 release. I will put up a PR.

My motivation has less to do w/ bundle size than ease of maintenance, not just for this monorepo itself, but Hub.js and any similar libraries built on the rest-js libraries.

I think optionalDependencies might be best for @esri/arcgis-rest-auth in any packages that don't directly import from it.

[patrickarlt]

@tomwayson you might want to wait for my build system PR 😄 it needs to touch every source file and test.

[patrickarlt]

@tomwayson I'm fine with merging request and auth. I think for 4.0 we should still publish auth and just have it re-export things from request for compatibility and remove it fully at 5.0.

[tomwayson]

OK, thx for the heads up on the build system PR.

FYI re: bundle sizes, my idea is that after #785 and #788 are done that we'd drop the UMD build altogether. Though maybe the reason you're citing those above is that they are the easiest way to get the relative output sizes of the packages.

re: merging request and auth, I'll take a look at that.

[patrickarlt]

I still think the UMD build is a good idea for now since it allows you to use ArcGIS REST JS with the JS API as AMD modules which I think is a valid use case, especially internally at Esri. Is is also still helpful over CDN because you can drop it into CodePens dependencies and while you CAN use ES modules (#788) I don't think people are super familiar with using them in a context like CodePen yet.

Maybe we do this:

• 4.0 Introduce Build ES modules for public CDN  #788 and start documenting ES modules and the main approach
• 5.0 Drop the umd build down to iife so we just have a single global. and that is it. No more Common JS or AMD for browsers. Just ESM, Common JS for Node, or a global.
• 6.0 Drop the iife build entirely and just do ESM for browsers and node and Common JS for Node.

If we feel like accelerating things we can always do

• 4.0 Introduce Build ES modules for public CDN  #788 and start documenting ES modules and the main approach
• 5.0 Drop the umd build entirely. People should be using ES modules by now.

There is some push back against switching everything to ESM only too soon (in Node) node-fetch/node-fetch#1263 and sindresorhus/meta#15.

[patrickarlt]

@alaframboise @ak-kemp Summarizing this for you here since this will be a major change in the doc at 4.0. We are going to merge @esri/arcgis-rest-request and @esri/arcgis-rest-auth. We will still publish @esri/arcgis-rest-auth for backward compatibility but we want to move everything to arcgis-rest-request only to improve maintainability of the library.

Current 3.x https://developers.arcgis.com/documentation/mapping-apis-and-services/search/geocoding/#geocode-an-address

import { geocode } from "@esri/arcgis-rest-geocoding"
import { APIKey } from "@esri/arcgis-rest-auth"

geocode({
  address: "1600 Pennsylvania Ave NW, DC",
  authentication: new APIKey("YOUR_API_KEY"),
}).then(response => {
  response.candidates[1].location // => { x: -77.036533, y: 38.898719, spatialReference: ... }
})

Future 4.0

import { geocode } from "@esri/arcgis-rest-geocoding"
import { APIKey } from "@esri/arcgis-rest-request"

geocode({
  address: "1600 Pennsylvania Ave NW, DC",
  authentication: new APIKey("YOUR_API_KEY"),
}).then(response => {
  response.candidates[1].location // => { x: -77.036533, y: 38.898719, spatialReference: ... }
})

[alaframboise]

@patrickarlt seems very odd to call a variable APIKey for a request module ref.. doesn't make sense to me.

When is this planned for? I was going to create a rest js/location services video soon but I'll shelf it.

[patrickarlt]

@alaframboise We are merging request and auth packages for a few reasons:

1. Improve maintainability, auth and request are pretty tightly coupled anyway. For example ArcGISAuthError is actually part of request.
2. Improve the dependency chain. Everything will rely on @esri/arcgis-rest-request so you only have 1 package to manage instead of 2

I don't really think import {APIKey} from "request" is super strange. There really isn't anything else to name it since it isn't really like the other *Session objects.

It is worth noting 2 additional things:

1. 4.0 will also include Allow authentication with only a token #889 so you can just pass the API Key as a string to the authentication (or maybe token) options:

   import { geocode } from "@esri/arcgis-rest-geocoding"
   
   geocode({
     address: "1600 Pennsylvania Ave NW, DC",
     authentication: "YOUR_API_KEY",
   }).then(response => {
     response.candidates[1].location // => { x: -77.036533, y: 38.898719, spatialReference: ... }
   })

2. This isn't a breaking change. We are still going to publish @esri/arcgis-rest-auth with the same exports and just deprecate it and point users to @esri/arcgis-rest-request so you should still be able to make videos for 3.x and the code and packages will still work in 4.x. I have no idea when 4.0 will launch since we still have a ways to go.

[alaframboise]

I don't really think import {APIKey} from "request" is super strange. There really isn't anything else to name it since it isn't really like the other *Session objects.

You don't get an APIKey back from a declare.

I'm find with everything else.

[patrickarlt]

You don't get an APIKey back from a declare.

@alaframboise I don't understand can you elaborate on this?

[patrickarlt]

@alaframboise you mean that if you do:

const myAPIkey = new APIKey("AAPK...");

You would expect to get a new API key item not create a session object for request

[alaframboise]

@patrickarlt yes exactly. If I create an instance of a request or location service class, it returns the class not an APIKey. We don't want to confuse developers. A request class and a key are two different things.

[noahmulfinger]

Just adding my two cents: Would it make more sense to rename APIKey to APIKeySession? That way it would be consistent with the other session objects, like UserSession and ApplicationSession.

[patrickarlt]

@alaframboise @noahmulfinger moving discussion about renaming the APIKey class to #914 since I think it is about more then that since we also have #889 and #893 in progress as well.

[patrickarlt]

I've merged request and auth in #911 (comment)