refactor: remove common-types entirely (and move typings into whatever package makes the most sense)

[jgravois]

while we're preparing to release v2.0.0, i'd like to take the opportunity to make common an actual (non-external) dependency of portal, feature-service, geocoder and routing, instead of an external peerDependency.

1. the bulk of the package is shared typings that will benefit TypeScript developers and be invisible to others.
2. the minified gzipped JS cost (right now) is only 10bytes.

3. non TS users will no longer see peerDependency warnings asking them to install another package they don't need.
4. we can start telling folks to:

npm install @esri/arcgis-rest-request && npm install @esri/arcgis-rest-routing

instead of

npm install @esri/arcgis-rest-request && npm install @esri/arcgis-rest-common && npm install @esri/arcgis-rest-routing

AFFECTS PACKAGES:
@esri/arcgis-rest-feature-service
@esri/arcgis-rest-geocoder
@esri/arcgis-rest-portal
@esri/arcgis-rest-routing

[tomwayson]

After consolidating all items/groups/sharing into -portal, there's only 2 fns in -common and they appear to only be used by -routing:

https://github.com/Esri/arcgis-rest-js/search?q=isLocationArray&unscoped_q=isLocation
https://github.com/Esri/arcgis-rest-js/search?q=isLocationArray&unscoped_q=isLocationArray

But -geocoder should be refactored to drop it's local implementations of those fns and import the shared ones.

My first question is that I'm not 100% sure that a TS project will automagically get the types of a transitive dependency. We'd need to test that before going ahead w/ this approach.

TBH - I'm a little more into the idea of going back to putting our common types in -common-types as a peerDependency. Sure the warning is annoying, but I think it's pretty obvious that it's only relevant to TS users, and easy enough for us to doc the typical pattern is to not install it and ignore the warning unless you are using TS.

My next question is, if someone needed to use the fns in -common directly, how would they use them? I don't think they are re-exported from -routing? Even if they are, once added to -geocoder would it re-export them too? Are docs are still generated for -common? If so, I'd guess someone would go and install that package, but then they'd have the fns twice. That may not be a big deal right now since this is only 10b, but what if we start adding more here?

Maybe we should get rid of -common (assuming we put the types back in -common-types) and create a package like -location-utils or something for these fns and having only -geocoder and -routing depend on that package. We could still make the new package a full-blown dependency of those so that people don't need to install it. To me that's much more clear that what we'd be doing w/ this PR.

[jgravois]

I'm not 100% sure that a TS project will automagically get the types of a transitive dependency. We'd need to test that before going ahead w/ this approach.

they definitely do. i confirmed by npm installing 'terraformer-arcgis-parser',
(which relies on @types/geojson).

-geocoder should be refactored to drop it's local implementations of those fns and import the shared ones.

i did that in #512

if someone needed to use the fns in -common directly, how would they use them?

they'd need to npm install it (or load its own umd).

what if we start adding more here?

that's the whole point. so far its just been a placeholder package and we've stuffed extra methods like getPortalUrl() and appendCustomParams() into request even though only the higher order packages take advantage of them.

[tomwayson]

Just to make sure I understand, that screen shot is from a package that installed terraformer-arcgis-parser but not @types/geojson, right?

Even if so, I don't like it (sorry). Ever since embroider-build/ember-auto-import#95 (comment) I've been following Edward Faulkner's philosophy of:

In general, I want anyone writing import x from "thing" to have "thing" in their own deps in package.json. Anything else is weird and hard to analyze both for humans and machines.

Which, as he says, leaves 2 options if you want to allow consuming packages to use what's in your own dependencies:

• use dependencies and re-export what you import
• use peerDependencies

That goes for both the types and the fns.

The whole reason we're doing this PR is to further reduce the number of peerDependencies, right? So I'd be fine w/ usingdependencies and re-exporting. We'd end up having the same fn exported by multiple packages, which I think is OK, but I'm not sure how our docs would handle this. You'd want them to show the re-exported fns but not doc the common package. How hard would it be to make that happen? Also, we'd have to remember to re-export any time we import from -common in any package. In general, I don't think we'll be adding a lot of fns to -common, but types might have more churn.

All in all, I wonder if sparing our users an extra npm i will be worth all of that extra burden that we'll be placing on the maintainers.

[jgravois]

how do you feel about making this change @patrickarlt?

[patrickarlt]

I'm with @tomwayson on this one. I've been bitten by the weird trick of requiring or importing something I didn't explicitly npm i and ended up with an older version that caused bugs for hours until I figured it out.

I don't think the extra burden of a peerDep install is all that bad.

@tomwayson it is worth noting that common will have withOptions() and the new version of appendCustomOptions() which will be pretty heavily used.

[tomwayson]

yeah, -common is turning out to be quite the little powerhouse.

If we want to get rid of one more npm i for people, then I think the best thing to do would be get rid of -common and move the fns and types into -request. Again we're talking about probably < 50b. I don't mind pushing that on the people that are only going to use -request. Furthermore, just because they only install -request doesn't mean that they will only use request(). If they're effectively rewriting what we have in the other packages, there's a good chance that they'll need getPortalUrl() or withOptions(), or even appendCustomOptions() down the line.

[jgravois]

fair enough. my preference would be to:

1. put appendCustomParams, withOptions and our two utility functions for routing and geocoding inside request and let tree-shaking do its job.
2. go back to calling it common-types
3. advertise common-types as a peer that only TS devs need to install themselves.

if that works for you @patrickarlt, just say the word and i will make it so.

[tomwayson]

I'd also be fine w/ putting the types in -request since there's no cost to JS consumers, but could just as easily do as you propose and go back to -common-types.

[jgravois]

the only reason i'm not keen on placing the types in request is because it'd make the API reference for that package so unwieldy.

[noahmulfinger]

Just thinking out loud, but what if -request was just renamed to -common? Then it could hold common types, the request function, and any other utils. It would have to continue to be a peer dependency though.

[patrickarlt]

@jgravois sounds like a good idea to me. Maybe rename common-types to shared-types to differentiate it from the common package a little?

[jgravois]

Just thinking out loud, but what if -request was just renamed to -common?

for better or worse, some folks just want to make requests, so i think there's a benefit to making sure its in the name of our core package.

how about this?

• @esri/arcgis-rest-request - includes appendCustomParams, withOptions
• esri/arcgis-rest-common
• @esri/arcgis-rest-portal - includes getPortal, getPortalUrl
• @esri/arcgis-rest-types - makes it super clear to TS users (and everyone else) whats in the 📦

[patrickarlt]

I like @esri/arcgis-rest-types the most right now.

[noahmulfinger]

My only thought with arcgis-rest-types is that I would not expect to have to install a separate types package when the project is already written in typescript. But as long as we're explaining its purpose I think it works fine.

[patrickarlt]

@noahmulfinger do you think that we should re-export all common types from the respective packages then? That wouldn't add any weight to the builds and it would avoid a separate install?

For example in the new @esri/arcgis-rest-portal package we can just tack on:

export { IItem /* re-export all the types used in portal */ } from "@esri-arcgis-rest-types"

[patrickarlt]

@jgravois or do we just try to move everything out of common-types and into its respective package now that we are combining most things into portal? Do we really need a shared typing package anymore?

[jgravois]

Do we really need a shared typing package anymore?

i think we could get away without it. i'll take a look.

[noahmulfinger]

Trying to move stuff into the respective packages definitely seems like a better option than re-exporting types. I imagine it would be easy to forget exports, and it might complicate the doc build.

[jgravois]

alright, i've pushed up a couple more commits:

• removed 'getPortalUrl()' and 'getPortal()' from request and placed them in portal
• remove 'getUserUrl()' from auth and placed it in portal
• moved the location utils methods into both geocoder and routing
• removed common-types
• moved pretty much all the typings into request (you'd be surprised how many interdependencies there are).
• updated test import statements to utilize the raw TypeScript from dependent packages instead of relying on compiled esm

import { IPoint, ILocation } from "@esri/arcgis-rest-request/src";

i'm going to merge this work and then get #508 in sync with v2.0.0