search

Esri / arcgis-rest-js

compact, modular JavaScript wrappers for the ArcGIS REST API
https://developers.arcgis.com/arcgis-rest-js/
Apache License 2.0
349 stars 120 forks source link

v2.0.0 checklist #137

Closed jgravois closed 5 years ago

jgravois commented 6 years ago

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

dbouwman commented 6 years ago

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 commented 6 years ago

@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 https://github.com/Esri/arcgis-rest-js/issues/73 and https://github.com/Esri/arcgis-rest-js/pull/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:

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 commented 6 years ago

@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 commented 6 years ago

@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"

esri github io_arcgis-rest-js_api_feature-service_queryfeatures_ standard laptop 1

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 commented 5 years ago

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 commented 5 years ago

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 https://github.com/Esri/arcgis-rest-js/commit/a50da42b53f0968ac8e6045c96ffc88e16803839 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 commented 5 years ago

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 commented 5 years ago

Another item for a 2.0.0 would be to refactor ISearchRequestOptions.searchForm into ISearchRequestOptions and remove ISearchRequest. This was kind of covered in https://github.com/Esri/arcgis-rest-js/issues/104 but I think we should make it consistant with our discussions in https://github.com/Esri/arcgis-rest-js/pull/78.

patrickarlt commented 5 years ago

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 https://github.com/Esri/arcgis-rest-js/pull/506.

patrickarlt commented 5 years ago

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 https://github.com/Esri/arcgis-rest-js/pull/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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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.

patrickarlt commented 5 years ago

@jgravois I really like the idea of merging the admin methods into portal BUT since I really want to make a builder and some higher level admin functions does that make that package to heavy for Hub since Hub can't tree shake?

tomwayson commented 5 years ago

The feature service admin fns do somehow make more sense w/ portal than w/ feature-server, so if we have to merge -admin I'd say under -portal is OK, but I'd still rather leave it as it's own package.

We should not do anything for Hub's sake, b/c at the end of the day, Hub is going to need every fn in this repo.

However other Ember apps that need portal fns, but don't need to administer feature services would be burdened w/ the extra weight, and I don't think that's great.

There are about a dozen people in the world that need -admin, and half of them are on this thread, and I'm totally OK w/ burdening the others w/ one more npm i.

tomwayson commented 5 years ago

Since we've been touching on function signature consistency in a few of the 2.0.0 issues, and are talking about having request(url, options) become request(optionsWithUrl) in #508, I think we should enforce the idea that I had we discussed way back in https://github.com/Esri/arcgis-rest-js/pull/78#issuecomment-352160174 - all "fetch" functions should take a single argument.

Currently our convention is that if there is a way to call a fn w/o all the ceremony of IRequestOptions then we allow for multiple arguments w/ IRequestOptions being an optional last argument, for example getItem(). I'd rather see that be like geocode() where there is only one option of type string | IGeocodeRequestOptions and go from:

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

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

If y'all agree, I can take this on as a 2.0.0 task.

jgravois commented 5 years ago

i'm fine with leaving feature-service-admin a standalone package (with a dependency on portal).

all "fetch" functions should take a single argument.

i'm on board with that, but if we're really going to be consistent, request should support:

request(urlOrOptions: string | IRequestOptionsWithUrl)

and i don't know if that relieves @patrickarlt's compositional headaches or not.

tomwayson commented 5 years ago

I think that would be the ideal signature for request() but If causes headaches, I'm fine w/ request not accepting a url as a string.

patrickarlt commented 5 years ago

@jgravois @tomwayson I'll be honest I'm not really a fan of withUrl() at all anymore. I think we should just ship withOptions() and not ship withUrl() at all and let people do:

const queryTrees = withOptions({
   url: "trees layer url"
});

for methods they want to override the url option on. This leave it up to users how to resolve https://github.com/Esri/arcgis-rest-js/pull/508#issuecomment-482879646 for methods that don't accept options.url.

It simplifies what we have to ship and test and we don't have to change any method signatures unless we want to.

jgravois commented 5 years ago

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

that's a-okay with me.

itrew commented 5 years ago

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 commented 5 years ago

I have to be honest I'm not a super big fan on forcing the pattern in https://github.com/Esri/arcgis-rest-js/issues/137#issuecomment-482883019

// 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 https://github.com/Esri/arcgis-rest-js/pull/78#issuecomment-352124790 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 commented 5 years ago

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.

https://github.com/itrew/arcgis-rest-js/issues/1

tomwayson commented 5 years ago

@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:

https://github.com/Esri/arcgis-rest-js/blob/9da67f884d2737931109889fe6597269bd5d8137/packages/arcgis-rest-geocoder/src/geocode.ts#L82 https://github.com/Esri/arcgis-rest-js/blob/9da67f884d2737931109889fe6597269bd5d8137/packages/arcgis-rest-portal/src/items/search.ts#L47 https://github.com/Esri/arcgis-rest-js/blob/9da67f884d2737931109889fe6597269bd5d8137/packages/arcgis-rest-portal/src/users/get.ts#L38

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 commented 5 years ago

@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 https://github.com/Esri/arcgis-rest-js/issues/137#issuecomment-483383954.
tomwayson commented 5 years ago

@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 commented 5 years ago

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:

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 commented 5 years ago

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 commented 5 years ago

I would also like to loop back on our discussions in https://github.com/Esri/arcgis-rest-js/pull/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 commented 5 years ago

@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:

jgravois commented 5 years ago

I can get behind:

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

as for portal, before i drank the kool aid i had the same concerns (see https://github.com/Esri/arcgis-rest-js/issues/67#issuecomment-348026809). 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:

https://github.com/Esri/arcgis-rest-js/blob/a871246a9cb9293a882595fde7f1256a0fb1e7e7/packages/arcgis-rest-portal/package.json#L4

tomwayson commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

I'm fine with what @jgravois proposed in https://github.com/Esri/arcgis-rest-js/issues/137#issuecomment-484166864.

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

all v2.0.0 work items are complete.

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

itrew commented 5 years ago

@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 https://github.com/Esri/arcgis-rest-js/issues/137#issuecomment-485041871), is it possible to export interfaces that belong to the submodule Webmap?

jgravois commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

This was just an option try and keep things clean.

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