Closed tomwayson closed 2 years ago
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.
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.
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.
@tomwayson 2 updates to this:
@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.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 |
![]() |
![]() |
@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.
@tomwayson you might want to wait for my build system PR 😄 it needs to touch every source file and test.
@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.
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.
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 (https://github.com/Esri/arcgis-rest-js/issues/788) I don't think people are super familiar with using them in a context like CodePen yet.
Maybe we do this:
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.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
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) https://github.com/node-fetch/node-fetch/issues/1263 and https://github.com/sindresorhus/meta/discussions/15.
@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: ... }
})
@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.
@alaframboise We are merging request
and auth
packages for a few reasons:
auth
and request
are pretty tightly coupled anyway. For example ArcGISAuthError
is actually part of request
.@esri/arcgis-rest-request
so you only have 1 package to manage instead of 2I 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:
4.0 will also include https://github.com/Esri/arcgis-rest-js/issues/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: ... }
})
@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.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.
You don't get an
APIKey
back from a declare.
@alaframboise I don't understand can you elaborate on this?
@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
@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.
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
.
@alaframboise @noahmulfinger moving discussion about renaming the APIKey
class to https://github.com/Esri/arcgis-rest-js/issues/914 since I think it is about more then that since we also have https://github.com/Esri/arcgis-rest-js/issues/889 and https://github.com/Esri/arcgis-rest-js/issues/893 in progress as well.
I've merged request
and auth
in https://github.com/Esri/arcgis-rest-js/pull/911#issuecomment-917422870
Currently just about every package has 2
peerDependencies
: request and auth. In my experiencepeerDependencies
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):We should remove those dependencies.
Second, arcgis-rest-service-admin only
import
sIUserRequestOptions
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 underdevDependencies
, 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 theUserSession
class to the types package. We could move the dependency underdevDependencies
, but it would be up to us to make sure we don't introduce any runtime uses (likenew UserSession()
). Seems a bit risky. Maybe we just leave it as underpeerDependencies
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."