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

Improve discovery of server root when url is for "services" endpoint. #582

Merged
merged 1 commit into from
Jun 14, 2019
Merged

Improve discovery of server root when url is for "services" endpoint. #582

merged 1 commit into from
Jun 14, 2019

Conversation

kenlyon
Copy link
Contributor

@kenlyon kenlyon commented Jun 11, 2019

Previously it required a slash at the end. Now it works with pound,
question mark and end of string.

Previously it required a slash at the end. Now it works with pound,
question mark and end of string.
@codecov
Copy link

codecov bot commented Jun 11, 2019

Codecov Report

Merging #582 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #582   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          93     93           
  Lines        1366   1366           
  Branches      245    245           
=====================================
  Hits         1366   1366
Impacted Files Coverage Δ
packages/arcgis-rest-auth/src/UserSession.ts 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d2975a...39c11ef. Read the comment docs.

@tomwayson
Copy link
Member

tomwayson commented Jun 12, 2019

Thanks @kenlyon!

I would have expected the fix for your issue to just make the trailing / optional?

Maybe there's a use case that I can't imagine, but I don't think we need to support the other delimiters.

My personal preference would be to wait until someone asks us to support # or anything else at the end, and just make the / optional at this time.

@kenlyon
Copy link
Contributor Author

kenlyon commented Jun 12, 2019

Maybe there's a use case that I can't imagine, but I don't think we need to support the other delimiters.

My main concern was that we don't have a false positive on something like:
"https://<hostname>/arcgis/rest/servicesarenice"

Admittedly, I think any url fragment in that position that starts with "services" but then has following characters would probably not be referring to a resource that exists.

Still, if all we did was make the trailing slash optional, we make the match less reliable in that way. If we didn't care about "#" or "?", we could still have "/" or $ (end of string) which would still protect against that situation.

@kenlyon
Copy link
Contributor Author

kenlyon commented Jun 12, 2019

As I think about the context a little more, the "#" character would only be relevant for scrolling to a particular id in an html page. Although the default response format is html, I don't see any id attributes or <a name="foo"> elements returned by the services endpoint, so perhaps it would never be needed.

I think that ? might come up depending on how the request() method is used. Maybe the query string parameters could be specified via the params property of the request options although I expect you would also support something like "https://<hostname>/arcgis/rest/services?f=pjson".

I know from my own case that "end of string" was what followed my "services".

Anyway, all that to say that it could be safe to simplify the regular expression a bit for the expected usage, but edge cases or future use might reveal the limitations of it, so I tend to favour the more comprehensive implementation to avoid surprises down the road.

Totally up to you, though. :) Let me know what you'd like.

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.

You've convinced me!

@tomwayson tomwayson merged commit eabfb14 into Esri:master Jun 14, 2019
@tomwayson
Copy link
Member

Thanks again @kenlyon. This was released in v2.0.4

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.

2 participants