Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add more common types #121

Merged
merged 3 commits into from
Feb 24, 2018
Merged

add more common types #121

merged 3 commits into from
Feb 24, 2018

Conversation

jgravois
Copy link
Contributor

@jgravois jgravois commented Feb 23, 2018

porting a whole load of interfaces and types originally written by @JeffJacobson 🎉

AFFECTS PACKAGES:
@esri/arcgis-rest-common-types
@esri/arcgis-rest-geocoder
@esri/arcgis-rest-groups
@esri/arcgis-rest-items
batch-geocoder

BREAKING CHANGE:
no longer prefacing interface names with an I

maybe its just the ArcObjects phobia talking, but 5 minutes into this port i started to agree with @tomwayson. A quick look at the TypeScript Language Specification made it clear that its not a universally agreed upon convention.

i'm planning on tackling #104 soon so it makes sense to me to rip off the proverbial esri-loader band-aid here too and just release a 2.0.0 if everyone else agrees.

for now, the associated linter rule is turned off because we're still using the I outside common-types.

AFFECTS PACKAGES:
@esri/arcgis-rest-common-types
@esri/arcgis-rest-geocoder
@esri/arcgis-rest-groups
@esri/arcgis-rest-items
batch-geocoder

BREAKING CHANGE:
no longer prefacing interface names with an I
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 73ce0b8 on moar-types into 52caf83 on master.

@patrickarlt
Copy link
Contributor

patrickarlt commented Feb 23, 2018

@jgravois @tomwayson while prefixing interfaces with I isn't a universal standard by any means (it is borrowed from .Net) but think it makes sense to differentiate between classes in the JS API Extent, Point, SpatialReference and Item and the interfaces that we use in these libraries, IExtent, IPoint, ISpatialReference and IItem.

I would hate to end up with something like this:

import { Extent } from "@esri/rest-js-common-types";

require(["esri/Extent"], (Extent) => {
  // now you have 2 problems...
});

Prefixing with I quickly tells you that the thing you are looking at is an interface and not a class. Just because the TypeScript Guidelines discourage it is no reason not to do since the guidelines are for contributing to TypeScript not writing TypeScript code in general.

It also seems a little silly to push 2.0.0 over something so minor.

@Esri Esri deleted a comment from coveralls Feb 23, 2018
@Esri Esri deleted a comment from coveralls Feb 23, 2018
@Esri Esri deleted a comment from coveralls Feb 23, 2018
Copy link
Member

@tomwayson tomwayson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel too strongly one way or the other about the I prefix, but I think @patrickarlt makes a salient point about the name collisions w/ the JSAPI classes. I'm pretty sure you could get around that by importing as, but why make it harder on people (other than @jgravois) than it already is.

requestOptions
)}/community/groups/${requestOptions.id}/delete`;
const url = `${getPortalUrl(requestOptions)}/community/groups/${
requestOptions.id
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL - you can have line breaks w/in ${} in template literals w/o adding line breaks to the output.

…nterfaces

AFFECTS PACKAGES:
@esri/arcgis-rest-common-types
@esri/arcgis-rest-geocoder
@esri/arcgis-rest-groups
@esri/arcgis-rest-items
@tomwayson
Copy link
Member

If we don't rename the existing interfaces, then this would not have to be a breaking change, right?

If not, I say we lump these in w/ either a 1.0.3 release, or a 1.1.0 release w/ #115

tslint.json Outdated
@@ -8,6 +8,7 @@
"strict-type-predicates": false,
"ordered-imports": ["any"],
"only-arrow-functions": [false],
"object-literal-sort-keys": false
"object-literal-sort-keys": false,
"interface-name": [false, "always-prefix"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want to remove this now that we're back to Iing everything, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

definitely. fixed.

@tomwayson tomwayson changed the title add more common types (and stop preceding these interface names with an 'I') add more common types Feb 23, 2018
@tomwayson
Copy link
Member

This LGTM now, so merge and include in 1.0.3, or wait for 1.1.0?

@jgravois
Copy link
Contributor Author

merge and include in 1.0.3, or wait for 1.1.0?

I'm fine with either. i just want to make sure and include #120 in v1.0.3

@tomwayson
Copy link
Member

If 1.0.3 is eminent, I'd say let this be the first PR of 1.1.0

@tomwayson tomwayson merged commit 78af91f into master Feb 24, 2018
@tomwayson tomwayson deleted the moar-types branch February 24, 2018 05:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants