Address #343. Add GeoJSON types to feature service package.

[itrew]

Added GeoJSON output type to several methods in the feature service package.

AFFECTS PACKAGES:
@esri/arcgis-rest-feature-service

This is my first crack at adding some overloads and generics thanks to the suggestion by @tomwayson in #343. I'm not 100% on this, so please give it a good shake.

One thing I noticed is that it is still a little wonky with the f parameter if the type isn't explicit. For example, the first snippet errors and the second does not.

const requestOptions = {
      url: serviceUrl,
      id: 42,
      params: {
        f: 'geojson'
      }
    };
getFeature(requestOptions) <-- Errors because f: string is not assignable to type 'geojson'.

const requestOptions: IGetFeatureRequestOptions<IQueryGeoJSONFormatParams> = {
      url: serviceUrl,
      id: 42,
      params: {
        f: 'geojson'
      }
    };
getFeature(requestOptions) <-- Now works

Maybe there is a more graceful way to handle that? I'm all ears.

[itrew]

Oops, looks like it didn't import the GeoJSON type namespace. I'll take a look at fixing that later today.

[tomwayson]

Thanks for this @itrew!

Personally though, I think the overloads approach adds more cognitive overhead than it is worth. I could live w/ that if it worked well, but I'm wary of the 'geojson' isn't a string BS that you describe in #346 (comment)

Instead what if we just started by just adding to the union type that is currently returned and have the focus of the PR be on making sure that we get the geojson types added correctly (is it really sufficient to make it only a dev dependency?)

[itrew]

@tomwayson I think the overloading does work with minimal thought, but it's the generics that make it a little more cumbersome. They way I set it up, however, should be able to hide that if TS is able to infer the parameter types correctly.

Unfortunately it looks like the BS from #346 (comment) is more due to a limitation of the literal types used for the output formats doesn't match how TS infers strings. Based on the info here, there are a couple different solutions that seems pretty straightforward.

// Works out of the box. The overload defaults to a JSON format and returns the IFeature type.
const requestOptions = {
      url: serviceUrl,
      id: 42
};
getFeature(requestOptions)
    .then(response => {
          console.log(respone.attributes)
    }

// Throws a compile error because of the string inference.
const requestOptions = {
      url: serviceUrl,
      id: 42,
      params: {
        f: 'geojson'
      }
};
getFeature(requestOptions) <-- Error thrown here like example in previous comment.
    .then(response => {
          console.log(respone.attributes)
    }

// After a minor tweek to ensure you are inferring the right format type.
const requestOptions = {
      url: serviceUrl,
      id: 42,
      params: {
        f: 'geojson' as 'geojson'   <-- assert the string as the 'geojson' type. Could be 'json' as 'json' too.
      }
};
getFeature(requestOptions) <-- No more error.
    .then(response => {
          console.log(respone.attributes)  <-- A helpful error here, because .attributes does not exist on a GeoJSON feature.
    }

All of that being said, if you all would rather just return a union type, I can roll these updates back and go for that. I think you would lose the helpful type errors (like the missing attributes property in the third example) however.

As far as the dependency goes, you make a very good point. Based on the comment here I've moved it to a dependency of the package itself so that they types are installed for those who are using the library with TS.

There are still some issues with Karma and the GeoJSON typings that I need to looks at. It seems something with it trying to read the type definition file is causing Karma to error out on the builds, even though the test are passing with Jasmine.

[coveralls]

Coverage remained the same at 100.0% when pulling 9123eea on itrew:fix-types-for-feature-service into 2863b5f on Esri:master.

[coveralls]

Coverage remained the same at 100.0% when pulling 9123eea on itrew:fix-types-for-feature-service into 2863b5f on Esri:master.

[coveralls]

Coverage remained the same at 100.0% when pulling 9123eea on itrew:fix-types-for-feature-service into 2863b5f on Esri:master.

[tomwayson]

Thank you for taking the time to provide such detailed info @itrew.

To be clear, you've done awesome work here, and I don't want to you rollback anything yet.

My problem is w/ TS. This makes me not want to use string literals. No end user should need to know that they have to use f: 'geojson' as 'geojson'.

I'm guessing we can't do this w/o the string literals, b/c there's no way for TS to infer which type it should return, right?

To me, personally, the helpful type error (console.log(respone.attributes) <-- A helpful error here, because .attributes does not exist on a GeoJSON feature.) is not worth having to reply to a single issue w/ "yeah, the problem is, you need to use f: 'geojson' as 'geojson'. sorry."

B/c the only downside of returning a union type is that users won't get intellisense on response. unless they explicitly cast it to one of the known return types, right? If so, I'm OK w/ that. The benefits of TS are opt-in, but at least it doesn't put some BS type error in your face, b/c at the end of the day, it's perfectly fine to call this fn w/ f: 'geojson'.

That's just my 2 cents, but I'm curious how others like @noahmulfinger and @patrickarlt who actually consume this library from a TS app (I typically consume it from JS) feel.

If we decide to go w/ just returning union types, if it's easier to open another PR rather than roll back what you have, that's fine too.

[itrew]

I hear ya. It's been a little frustrating dealing with the string literals, but in a way they make sense. After reading this comment on a similar issue on the TS repo, I can see why it acts the way it does.

After some more thought, I think I'm going to scrap this PR for now and go with the union types. Another good reason to start fresh is that I just did some testing and apparently both the getFeature and queryRelated methods are unable to return a geojson. The feature endpoint doesn't support the geojson format, and the query related endpoint returns an error when attempting to retrieve the records in geojson. So I won't add the geojson typings to those methods in a new PR.

[itrew]

Closing for now. Moving the discussion back to #343