v2.0.0 checklist

[jgravois]

a list of breaking changes that we could group together. feedback welcome.

• drop the 'r' from arcgis-rest-geocoder?
• drop the 's' from arcgis-rest-items?
• drop the 's' from arcgis-rest-groups?
• rename arcgis-rest-common-types to arcgis-rest-common and move the growing collection of shared utility methods in request (that aren't actually needed by request) there along with the types
• getAsyncStuff> fetchAsyncStuff? (and use get for getters instead)
• consolidate feature-service and feature-service-admin
• start bundling common inside upstream packages?
• keep documenting the structure, but stop declaring a named interface, for unique responses.
• remove serviceInfo() from arcgis-rest-geocoder
• esriGeometryType > GeometryType
• esriUnits > Units
• esriFieldTypes > FieldTypes
• folder > folderId in IItemCrudRequestOptions (see createItemInFolder expects a folderId, not a folder name #236)
• fix capitalization of IOauth2Options (to be IOAuth2Options)
• replace "adds"/"updates"/"deletes" in feature service crud functions with consistent use of "features"
• remove appendCustomParams() re-export from feature-service (see ensure 'first class citizen' geocode request parameters are passed through #365)
• change signature of getItemResources() (for consistency)
• remove IGeocodeParams re-export IGeocodeParams (until v2.0.0) #468
• remove serializeGroup()fix: deprecate serializeGroup() and simplify SerializeItem() #475
• remove IItemResourceAddRequestOptions
• consolidate items, groups, sharing and users packages into @esri/arcgis-rest-portal
• IParamBuilder / SearchQueryBuilder
• rename common-types to types and rexport typings, make it a real dep, and rexport interfaces and types from dependent packages
• move getPortalUrl() and getPortal() from request to portal
• move getUserUrl() from auth to portal
• change signature of appendCustomParams
• add withOptions() and setDefaultOptions() to request
• make signatures of searchItems() and searchGroups() consistent Inconsistent signatures... #104
• generateToken(optionsOrParams) > generateToken(options) (see: fix(:bug:): support use in Node.js with custom fetch (and no global) #264 (comment))
• geocoder > geocoding @tomwayson
• feature-service > feature-layer (@tomwayson)
• feature-service-admin > service-admin (@tomwayson)
• re-exporting types in upstream packages (@patrickarlt)
• standardize exported constants (@patrickarlt)
• ICustomeRequestOptions > ICustomOptions @tomwayson

[dbouwman]

General thoughts...

Currently this API feels more complex than it is. I think this is partly from the use of inherited/extended interfaces, and a tendency toward "elegance" over "transparency"

Many functions take a single argument, that is ISomethingRequestOptions which extends ISomethingElseRequestOptions which extends IRequestOptions, which requires .authorization which is IAuthenticationManager which is implemented by two different classes.

While elegant, in order for a user to know what they need to pass into this fn, they need to look at 3 different doc pages.

Whereas, we could make the useage of many of these method more transparent if we simply passed in more, well-named, arguments, instead of collapsing the args into an ISomethingRequestOptions.

As @tomwayson noted - perhaps some of this could be addressed by using TypeScrip union types or overloading, and may not require a 2.x release

[patrickarlt]

@dbouwman I feel like you are conflating up a problem with the docs vs a problem with the library. We all argued endlessly about multiple function params vs a single object params in #73 and #78 and ultimately we all agreed that the single object param was probably the best way to go because it is to difficult to figure out what should be broken out into a standalone param and what should be moved into an optional request options object.

Managing a single options object requires a lot less work on our part and allows for non-breaking changes more easily then hardcoding lots of options in distinct params.

If we need to solve the issue of having to look at several doc pages:

• The actual method
• The IWhateverOptions
• IRequestOptions

That is an issue with the doc build that I can work to integrate IWhateverOptions and IResponseOptions into the method pages. I don't think we need to go back to refactoring all the methods all over again to resolve this. I feel like there are just to many variables to figuring out even the most basic methods like moving items between owners or sharing items to figure out solid method signatures.

[dbouwman]

@patrickarlt I can say that from using the library, the current approach is cumbersome, regardless of the doc structure. I totally agree that this is a hard problem to solve, given the API we are abstracting, and all the use-cases we are trying to support... so maybe the single-object-params solution is the best we can do... it's just been O_o when working with this api. If ya'll have been using it on other projects, and feel it works as-is, then we can shelf this.

[patrickarlt]

@dbouwman I haven't contributed in awhile but while digging through the docs today I think one area we could really improve is how we use the options.params key in the request options.

I originally intended options.params to be an escape hatch that would allow users to pass ANY param that we didn't wrap for them.

However it seems like options.params is also being subclassed so we can document every param which I don't think we should do. For example queryFeatures takes a single IQueryFeaturesRequestOptions which in turn has a params key with a value of IQueryFeaturesParams which simply has EVERY possible param for /query in it.

It would be simpler for users if IQueryFeaturesRequestOptions could simply accept any param for /query or at least the most common ones. Users can still user IRequestOptions.params to add any params not wrapped in IQueryFeaturesRequestOptions. As a practical example I think we should do this:

export interface IQueryFeaturesRequestOptions extends IRequestOptions {
  where?: string;
  // ect...
}

Then in queryFeatures:

// start with any user defined params from the options.params literal.
const params = options.params;

// sometimes options will override params.
params.where = options.where || '1=1';

This means that that IQueryFeaturesRequestOptions.params is just an object literal of params that we DO NOT DOCUMENT i.e. use the REST API docs and IQueryFeaturesRequestOptions is something we specifically want to make easier for users.

With changes like this im place we can do the following:

1. Add a "Options" section to methods that take a single interface as a param
2. Inline that interface definition under "Parameters"

I like this approach where we don't wrap every param. Instead lets just mention:

You can also add any parameter in link to the REST API doc in options.params, see an example.

This means that:

1. We still subclass IRequestOptions but just to add custom properties never to override params.
2. Cleans up the doc in an easy way.
3. We still get to use a single object as params which is REALLY nice from an API perspective.

@dbouwman we used this library a whole bunch in the Vector Tile Style Editor and I generally wasn't too annoyed when I had to click though 1 page to get tot he key options for the request. But when I had to click through twice (method > options > params) it got annoying.

I also think we should also do what @jgravois suggested and short required params to the top and inherited params to the bottom and look into some way to sort popular params to the top.

[tomwayson]

Before we go to 2.0 I'd like to at least discuss consolidating all the "portal" packages - items, groups, sharing, users, etc, are all so intertwined that even if you start out only using 1 or 2 of them, you'll eventually end up using most of them. I think it'd be a lot simpler if those were all in @esri/arcgis-rest-portal (or -sharing, or whatever). Now that modern apps can treeshake our packages (#490), we can stress less about bundling too much code.

[patrickarlt]

Now that tree shaking is working I think package consolidation makes a lot of sense for a 2.0.0. If you work with 1 method you are probally going to work with a bunch of methods. Having @esri/arcgis-rest-portal makes a lot of sense since it would combine 4 modules down to 1. I think we should also move things like getPortal() and getPortalUrl() out of @esri/arcgis-rest-request and into a new @esri/arcgis-rest-portal package.

I also think we could consolidate @esri/arcgis-rest-feature-service and @esri/arcgis-rest-feature-service-admin since modern apps can now treeshake. To further reduce the number of packages.

I would also like to refactor the commits that @jgravois made in a50da42 and see if I can get IFetchTokenParams IGenerateTokenParams and ITokenRequestOptions moved back to @esri/arcgis-rest-auth. I think I might be able to do it with Union types.

[patrickarlt]

Combining everything into @esri/arcgis-rest-portal also solves the issue of where to put SearchQueryBuilder since it would be weirdly shared between only @esri/arcgis-rest-items and @esri/arcgis-rest-groups.

[patrickarlt]

Another item for a 2.0.0 would be to refactor ISearchRequestOptions.searchForm into ISearchRequestOptions and remove ISearchRequest. This was kind of covered in #104 but I think we should make it consistant with our discussions in #78.

[patrickarlt]

I also think it makes sense to cut a 2.0.0 soon with all these changes since we realized we should have anyway because of #506.

[patrickarlt]

I'm also going to propose that we make request no longer able to accept a url as the first param to solve some of the issues with #508. request would be infinity more flexible if it simply accepted IRequestOptions as its only param. Since most other methods make you specify the url in their params anyway I don't think this is a big deal.

[tomwayson]

I think we should also move things like getPortal() and getPortalUrl() out of @esri/arcgis-rest-request and into a new @esri/arcgis-rest-portal package.

getPortal() ✔️
getPortalUrl() - this is used in -auth and -feature-service-admin, so I don't know if we can do that.

I also think we could consolidate @esri/arcgis-rest-feature-service and @esri/arcgis-rest-feature-service-admin

I'm less inclined to do that just b/c I think so few people need -admin, but I don't feel strongly that they should be kept separate.

[tomwayson]

Combining everything into @esri/arcgis-rest-portal also solves the issue of where to put SearchQueryBuilder

I agree, but I could see another way to do it would be to put SearcQueryBuilder & friends into a higher level package that sits on top of of, or may even be independent of the other rest-js packages. Consumers could call .toParam() and pass the output of that into the rest-js fns instead of passing an instance of the builder.

[jgravois]

I'd either leave getPortalUrl in request or move it into common.

if we're consolidating so much into portal I think it'd be a mistake not to consolidate feature-service too.

[patrickarlt]

@jgravois I feel like feature services should still be their own package since I think lots of people will use them independent of the portal packages and conceptually they related to a very different API area.

[jgravois]

I've been thinking about it quite a bit and i'm starting to think that it makes more sense to put the two methods we have for administering feature service endpoints into portal than feature-service.

• it consolidates it's own dependencies
• it separates something that it is used seldom from things that are used much more often
• their calls are made to a totally different API.

[jgravois]

I think we should just ship withOptions() and not ship withUrl() at all

that's a-okay with me.

[itrew]

If you all are thinking about 2.0, I have some work that you may or not be interested in. Several months ago I did some experimenting with TS generics and the geometry types of this lib. Now it's been a while, so I will have to refresh my memory, but the idea was to make the types more extensible. Below is a snippet from the write-up I did here. I went into much more detail in that thread.

// Create a polygon.
const examplePolygon: IPolygon = {
  rings: [[0, 0], [0, 1], [1, 1], [1, 0], [0, 0]],
  spatialReference: {
    wkid: 3857
  }
}

// Adding the polygon to a feature set with generic guarding.
// The third generic specifies that the feature set should contain point features.
const exampleFeatureSetWithGuard: IFeatureSet<any, any, IPoint> = {
  // Error: Type 'esriGeometryPolygon' cannot be assigned to type 'esriGeometryPoint'.
  // This is due to a conditional type that assigns the correct geometryType string to the
  // specified feature set geometry generic.
  geometryType: 'esriGeometryPolygon',
  spatialReference: {
    wkid: 3857
  },
  fields: [],
  features: [
    {
      // Error: Type IPolygon cannot be assigned to type IPoint.
      geometry: examplePolygon,
      attributes: {}
    }
  ]
}

Being several months old it isn't really inline with the current state of this project, but if it's something you are interested in I could work on incorporating it.

[patrickarlt]

I have to be honest I'm not a super big fan on forcing the pattern in #137 (comment)

// v1.x, not great  
getItem(id: string, requestOptions?: IRequestOptions) {};

// v2.x, better:
getItem(idOrOptions: string | IGetItemOptions) {};

I think the pattern we laid out works just fine. @tomwayson proposal would basically require us to make a custom options interface for every method. I do agree that the proposed getItem(idOrOptions: string | IRequestOptions) {}; WOULD work great with withOptions() but I think that it forces a functional programming pattern on users who might not want, need or understand it.

I'm still going to push for something like #78 (comment) like this:

interface IGeocodeRequestOptions extends IRequestOptions {
  address?: string;
  outSr?: number;
  // ect...
}

/**
 * Used to merge an array of options objects
 */
function mergeOptions<T>(...options) : T {
  let finalOptions = {};
  let o;

  while(o = options.shift()) {
    finalOptions = {...finalOptions, ...o}
  }

  return finalOptions as T;
};

/**
 * Higher order functions that returns a function where the first param will be 
 * passed as `key` and the remaining params are options objects for `mergeOptions`
 */
function mergeOptionsWith<T>(key: keyof T) {
  return function (helper: any, ...options: T[]) {
    if(helper && typeof helper === "object") {
      return mergeOptions<T>(...options, helper);
    }

    return mergeOptions<T>(...options, {[key]: helper});
  }
}

/**
 * functions now use overloads to handle their different possible implimenations
 * but all the logic around the special first param is handled by mergeOptionsWith
 */
function geocode(address: string, options?: IGeocodeRequestOptions): void
function geocode(options: IGeocodeRequestOptions): void
function geocode(address: string|IGeocodeRequestOptions, options: IGeocodeRequestOptions = {}): void {
  const defaultOptions: IGeocodeRequestOptions = {
    outSr: 4326
  };

  options = mergeOptionsWith<IGeocodeRequestOptions>("address")(address, defaultOptions, options, );

  console.log(options);
}

geocode('221B Baker St, London');
// { outSr: 4326, address: '221B Baker St, London' }

geocode('221B Baker St, London', {
  outSr: 102100
});
// { outSr: 102100, address: '221B Baker St, London' }

geocode({
  address: "221B Baker St",
  outSr: 102100
});
// { outSr: 102100, address: '221B Baker St' }

This pattern is definitely more implementation work BUT still allows for really clean signatures doesn't force functional programming patterns on users and we can abstract most of the implimentation work out to the mergeOptions() and mergeOptionsWith() helpers.

[jgravois]

Being several months old it isn't really inline with the current state of this project, but if it's something you are interested in I could work on incorporating it.

thanks for chiming in @itrew! to be honest, besides shuffling some code around, this is one area where we haven't put in much work because frankly my own team isn't juggling a lot of code that manages geometries or feature sets in production.

our goal is to ship a v2.0.0 late this week, so if you can find the time to put together a pull request (based on the branch of the same name), this would be an ideal time to make a few breaking changes to the types if they've proven helpful to you.

if he's interested, i'd be curious to get @JeffJacobson's take on your missive as well.

itrew#1

[tomwayson]

@tomwayson proposal would basically require us to make a custom options interface for every method

I didn't think of that. I still think in JS, not TS.

Looking at the 2.0.0. branch I see 8 fns w/ an optional second argument of type IRequestOptions (i.e. search on ?: IRequestOptions). For 6 of those fns the first arg is id: string, for 1 fn it is url: string, and the other fn is searchGroups(), which will be handled in #104. That's only a couple new interfaces (IRequestOptionsWithId, IRequestOptionsWithUrl), which I'm sure already exist under different names and/or could be the base from which most other options interfaces extend.

My main concern is consistency in the API, and the 2 arg fns in -portal just feel different to me than the rest of the API.

For example, when I was writing queryFeatures(), technically you could call it w/ just a url, so I wanted to write it as queryFeatures(url: string, options: IQueryFeaturesRequestOptions) to be consistent w/ the fns in -portal. However every other fn in -feature-service was going to require options w/ a .url. In the end it seemed more important to be consistent w/in the package than to be consistent w/ the -portal fns.

But then there are a few other places where we have a fn that takes only one argument that can be either a string or some kind of options hash:

arcgis-rest-js/packages/arcgis-rest-geocoder/src/geocode.ts

Line 82 in 9da67f8

address: string | IGeocodeRequestOptions

arcgis-rest-js/packages/arcgis-rest-portal/src/items/search.ts

Line 47 in 9da67f8

search: string | ISearchRequestOptions

arcgis-rest-js/packages/arcgis-rest-portal/src/users/get.ts

Line 38 in 9da67f8

requestOptions?: string | IGetUserRequestOptions

Each of those has sort of special conditions why the 2 arg format won't work as well. We could definitely use that format for queryFeatuers(urlOrOptions: string | IQueryFeaturesRequestOptions), and I think it could work for the 8 locations where we use 2 args if we add a couple of interfaces.

The single arg that's either a string or a options hash seems to be the one format that can work for every "fetch" fn we have.

[patrickarlt]

@tomwayson allowing queryFeatures(url | IQueryFeaturesRequestOptions) is going to cause major headaches for withOptions. Consider queryFeatures(url | IQueryFeaturesRequestOptions) vs queryFeatures(query | IQueryFeaturesRequestOptions):

const queryStates = withOptions({
   url: "..."
}, queryFeatures);

queryStates() // 1=1
queryStates("STATE_NAME = 'Alaska'")

works great. But this is what it would look like withqueryFeatures(url | IQueryFeaturesRequestOptions)

const queryStates = withOptions({
   url: "..."
}, queryFeatures);

queryStates()
queryStates({query: "STATE_NAME = 'Alaska'"})

I would always prefer a pattern where we pass a url in the options. I guess I'm still attached to the nice looking method signatures where we can pull out a single option. I think there 3 ways forward here:

1. Continue to be kinda inconsistent here we have some things that are fn(thing: string | CustomOptions), fn(thing: string, options?: IRequestOptions) and fn(options: CustomOptions).
2. Only allow fn(options: CustomOptions). Ever.
3. Only allow fn(options: CustomOptions) and fn(thingFromOptions: string, options: CustomOptions) where thing can never be the URL in line with v2.0.0 checklist #137 (comment).

[tomwayson]

@patrickarlt I'll admit that queryStates("STATE_NAME = 'Alaska'") looks great, but the thing is queryFeatures(whereOrOptions: string | IQueryFeaturesRequestOptions) would never work. You could not call queryFeatures("STATE_NAME = 'Alaska'") i.e. w/ just where and not a url. url is the one required option, that's why I want to make it the thingFromOptions.

Now that we've merged all the portal packages and aligned the signatures for searchItems() and searchGroups(), I think a consistent pattern is starting to emerge. The only place we use the 2 argument pattern is for fns that take ids in -portal, w/ one exception in -feature-service. I am going to PR to change that fn to make it consistent w/ the rest of that package.

I'll drop the whole notion of allowing queryFeatures(urlOrOptions: string) for now, b/c: a) I don't think it's too common for people to do 1=1 queries, and b) it can always be added later w/o a breaking change.

[tomwayson]

I'm really happy w/ the new structure of the packages. I think we've got everything in it's right place.

I do wonder a bit about the package names (sorry). I doubt @patrickarlt wants to revisit the great ontology debates that went into the section headings of https://developers.arcgis.com/rest/, but I have these concerns:

• for some people "portal" means "on premise" and not ArcGIS Online
• I think it should be -geocoding instead of -geocoder to be consistent w/ -routing and https://developers.arcgis.com/rest/location-based-services/
• should -feature-service-admin be -service-admin?
• should -feature-service be -features for brevity's sake, or maybe go the other way and make it more generic (like -services) in case we need to add fns that are not necessarily specific to feature layers like identify() or find()?

As far as the -portal issue, we punted on that for the REST docs by using "Users, Groups, and Items" (the same packages we merged into -portal). Maybe -content?

If any name changes result from this comment, I'll make the changes myself to atone for even bringing this up.

[patrickarlt]

for some people "portal" means "on premise" and not ArcGIS Online

True. -content works for me.

I think it should be -geocoding instead of -geocoder to be consistent w/ -routing and https://developers.arcgis.com/rest/location-based-services/

Also true.

should -feature-service-admin be -service-admin

Disagree since the only methods are specific for feature services

should -feature-service be -features for brevity's sake, or maybe go the other way and make it more generic (like -services) in case we need to add fns that are not necessarily specific to feature layers like identify() or find()

I would prefer -features and features-admin to -services. I Would honestly also prefer feature-layer to feature-service since most users think of these things as layers anyway even though the REST API thinks of them as services.

[patrickarlt]

I would also like to loop back on our discussions in #515.

After working with the new types exported from @esri/arcgis-rest-request I really don't like it at all. I'm importing types from a module where nothing related to that type is happening and I only installed it because I was told i has to by peer deps.

I think would rather do the following:

1. Move all the shared types to @esri/arcgis-rest-types
2. When a module like portal uses a type like IItem it should also re-export it.

This lets users:

1. Install all the extra types if they want to with @esri/arcgis-rest-types
2. Use types from generally the correct related module import {addItem, IItemAdd} from @esri/arcgis-rest-portal;

This feels way better then:

import { addItem } from @esri/arcgis-rest-portal;
import { IItemAdd } from @esri/arcgis-rest-request;

I'll volunteer for this since I feel strongly about it if everyone else agrees.

[patrickarlt]

@jgravois a minor nit for 2.0.0.

We should standardize our exported constants like worldRoutingService, worldGeocoder and NODEJS_DEFAULT_REFERER_HEADER. I think we should do:

• NODEJS_DEFAULT_REFERER_HEADER
• ARCGIS_ONLINE_ROUTING_URL
• ARCGIS_ONLINE_GEOCODING_URL

[jgravois]

I can get behind:

• geocoder > geocoding
• feature-service > feature-layer
• feature-service-admin > service-admin

pat is right about the methods in there now, but besides being concise, @tomwayson is being forward looking

• re-exporting types in upstream packages (if pat does the work)
• standardizing our exported constants

---

as for portal, before i drank the kool aid i had the same concerns (see #67 (comment)). content doesn't sit right with me either though because the package includes everything under search/ and community/ too.

given that the official doc punts to "Users, Groups and Items", i don't think there is a hope in hell of us landing on something more elegant, especially since we are prepending everything with @esri/arcgis-rest.

to put it another way, portal isn't great and it puts a burden on us to clarify everywhere else that we mean. but i can live with it. i already took a crack at how to demystify it with this slug:

arcgis-rest-js/packages/arcgis-rest-portal/package.json

Line 4 in a871246

"description": "ArcGIS Online and Enterprise content and user helpers for @esri/arcgis-rest-request",

[tomwayson]

i don't think there is a hope in hell of us landing on something more elegant

Agree, and I feel better about -portal after doing this experiment.

Search for "arcgis portal" - the results are all about enterprise. 😞

However, if you search for "arcgis portal rest" or "arcgis portal api - at least the first result goes to the Developers REST docs, and the rest are interleaved between that and enterprise API docs.

To me that means the concept of a "portal (REST) API" means items, groups, and users and applies to both Online or enterprise.

I can live w/ that.

[tomwayson]

@patrickarlt

After working with the new types exported from @esri/arcgis-rest-request I really don't like it at all. I'm importing types from a module where nothing related to that type is happening and I only installed it because I was told i has to by peer deps.

I don't know that our users will know or care that "nothing related to that type is happening".

Another idea is to leave them in -request but rename that package to -core.

Regardless of where the types end up (I don't really care if they stay in -request/-core or move to their own package called -types, or -common-types, or -shared-types), I'm not keen on re-exporting them b/c the same type will exposed and doc'd under multiple packages, leaving people (and maybe even tools) to wonder about the difference between the IPoint that comes from @esri/arcgis-rest-features and the one that comes from @esri/arcgis-rest-geocoding.

[patrickarlt]

I'm fine with what @jgravois proposed in #137 (comment).

I think -portal is the best we can hope for. Hopefully we can handle a page explaining portal on the Developers site someday.

I'm going to handle a -types package and re-exporting and i can handle the constants as well.

[itrew]

So @JeffJacobson and I briefly discussed the idea of splitting the types in order to differentiate between the common data types and webmap spec. For example, the IField interface are different between the two (webmap more or less extends it).

// common - field that belongs to a feature set
interface IField {
  name: string;
  type: FieldType;
  alias?: string;
  length?: number;
}

// webmap spec
export interface IField {
  name: string;
  type: FieldType;
  alias?: string;
  domain?: IDomain;
  editable?: boolean;
  exactMatch?: boolean;
  length?: number;
  nullable?: boolean;
  defaultValue?: any;
}

I see two ways of splitting them if that's something you're interested in.

1. import all of the webmap spec types and then rexport them as a named module of the types module. This would then require you to use the namespace when ever referencing those types.

//arcgis-rest-types/src/index.ts

import * as Webmap from './webmap';
export * from './feature';
// other top level exports
export { Webmap };

import { Webmap } from '@esri/arcgis-rest-types';

let fields: Webmap.IField[];

or 2) Any webmap specific type/interface is prepended with a 'WM' (e.g. IWMField). Not a huge fan of this.

Thoughts? I can spare some time in the morning getting something going but I'm not sure when you guys plan to pull the trigger on 2.0.

[jgravois]

option #1 sounds good to me

For example, the IField interface are different between the two (webmap more or less extends it).

unless there's an actual conflict between the two, we can also just be lazy and (only) document the webmap version even though we're not doing anything with webmaps here yet.

[itrew]

What was the idea behind re-exporting the types when used in a package when the package is already a dependency? I'm running into a little bit of a road block in trying to implement that when re-exporting interfaces under a Webmap named module exported by -types.

Going off of option 1 above, Webmap would have an interface IFeatureLayerDefinition that would be used in feature-service like so:

import { Webmap } from "@esri/arcgis-rest-types";

let definition: Webmap.IFeatureLayerDefinition;

If the idea is to re-export used types, how would we export Webmap.IFeatureLayerDefinition?

[jgravois]

What was the idea behind re-exporting the types when used in a package when the package is already a dependency?

i think its the other way around. @esri/arcgis-rest-types will now be a dependency of @esri/arcgis-rest-feature-layer (sorry for all the new names).

since feature-layer re-exports everything it uses from types, developers can do this:

import { IFeature, queryFeatures } from "@esri/arcgis-rest-feature-layer";

instead of

import { queryFeatures } from "@esri/arcgis-rest-feature-layer";
import { IFeature } from "@esri/arcgis-rest-types";

which doesn't preclude TS developers from importing from types. they'll actually still need to do that if they want to access stuff like Webmap that feature-layer doesn't use under the hood.

does that make sense?
does it work for your use case?
does it go with the grain of TypeScript?

[patrickarlt]

@itrew The reason for having a types package is that we will almost certainly need to share some types between pacakges. For example IUser is exported from -portal, -auth and -types so all 3 of these are valid:

import { IUser } from "@esri/arcgis-rest-auth";
import { IUser } from "@esri/arcgis-rest-portal";
import { IUser } from "@esri/arcgis-rest-types";

However in order to avoid having duplicate definitions of types we centralize them in -types and re-export them in every package where they are used.

In your example:

Webmap would have an interface IFeatureLayerDefinition that would be used in feature-service like so:

-types export BOTH IFeatureLayerDefinition and IWebmap separately:

//IFeatureLayerDefinition.ts
export interface IFeatureLayerDefinition {
  //....
};

//IWebmap.ts
import {IFeatureLayerDefinition} from `./IFeatureLayerDefinition`;
export interface IWebMap {
  //... do something with IFeatureLayerDefinition
};

// @esri/arcgis-restypes index.ts
export * from `./IFeatureLayerDefinition`;
export * from `./IWebmap`;

When you need to use IFeatureLayerDefinition you would do:

// some file somewhere
import { IFeatureLayerDefinition } from `@esri/arcgis-rest-types`;

// use IFeatureLayerDefinition

// index.ts
export { IFeatureLayerDefinition } from `@esri/arcgis-rest-types`; // re-export so users of `feature-service` can use IFeatureLayerDefinition with explictly requiring `-types`

[jgravois]

all v2.0.0 work items are complete.

i'm happy to continue the TypeScript discussion here or in a new issue.

[itrew]

@jgravois I've been chewing on this for a couple of days and I am still on the fence. It makes sense to me, but on one hand I just don't really see the benefit. Does it add anything other than removing a few import statements and one -types dependency for TS developers? If you weren't publishing -types it would make a lot more sense. The downside to me is now I need to double check that IFeature exported from @esri/arcgis-rest-types is in fact the same type that is exported by @esri/arcgis-rest-feature-layer. As of right now it's only documented once, so that becomes a little tricky.

On the other hand, I will say having every type encapsulated in each library that uses it is pretty elegant when only using one or two packages. It's ultimately your call though, just my 2 cents.

@patrickarlt I totally understand the reasoning behind managing your types in a single package. It would be a nightmare trying to maintain the same types in each package that uses them.

As for my example, I wasn't implying that Webmap was an interface, but rather it's own module with interfaces like IFeatureLayerDefinition. The thought was to create some sort of namespace separation between different ESRI types defined as common-data-types and the webmap spec. Some types like Field and FeatureSet have different shapes depending on which spec you are looking at. Server has responses with types from both specs depending on the endpoint.

import { IFeatureSet, Webmap } from "@esri/arcgis-rest-types";

// In this case, IFeatureSet !== Webmap.IFeatureSet as they are slightly different shapes.

So back to my original question. If you still plan to re-export used types and if we used a submodule Webmap to differentiate types (if @jgravois still thinks that makes sense per #137 (comment)), is it possible to export interfaces that belong to the submodule Webmap?

[jgravois]

Does it add anything other than removing a few import statements and one -types dependency for TS developers?

nope. thats about it.

The downside to me is now I need to double check that IFeature exported from @esri/arcgis-rest-types is in fact the same type that is exported by @esri/arcgis-rest-feature-layer

we're not manipulating any interfaces or types from @esri/arcgis-rest-types in any other packages. i don't think there's any need to be concerned about transitive dependencies changing from underneath you because lerna is going to bump the underlying dependent version of types in every other package automatically in each release to keep it in sync.

that said, you can also explicitly depend on, and import directly from, types.

is it possible to export interfaces that belong to the submodule Webmap?

we don't have any need to import/re-export Webmap in any other package at the moment. If it would be helpful for you if Webmap was added as a submodule within types to document types that extent their counterpart outside of webmaps would be helpful, i'd be happy to try and help you (or anyone else) land a PR.

[itrew]

we're not manipulating any interfaces or types from @esri/arcgis-rest-types in any other packages. i don't think there's any need to be concerned about transitive dependencies changing from underneath you because lerna is going to bump the underlying dependent version of types in every other package automatically in each release to keep it in sync.

I didn't mean to imply you were, I more meant it would be nice to have those re-exported modules documented as well (at least that it's coming from -types).

we don't have any need to import/re-export Webmap in any other package at the moment. If it would be helpful for you if Webmap was added as a submodule within types to document types that extent their counterpart outside of webmaps would be helpful, i'd be happy to try and help you (or anyone else) land a PR.

This was just an option try and keep things clean, not really a requirement of mine. Right now, this library has types from both specs intermingled (e.g. IField is from the webmap spec, IFeatureSet is from common). Now probably 95% of the time that won't be an issue, so it probably won't be an issue and can wait. Down the road though I'd be happy to assist with any typings.

[jgravois]

This was just an option try and keep things clean.

I agree. I think having organized webmap typings would be awesome.