-
Notifications
You must be signed in to change notification settings - Fork 13
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
search package #152
search package #152
Conversation
Codecov Report
@@ Coverage Diff @@
## master #152 +/- ##
======================================
Coverage 100% 100%
======================================
Files 57 86 +29
Lines 897 1233 +336
Branches 117 185 +68
======================================
+ Hits 897 1233 +336
Continue to review full report at Codecov.
|
Can we add a README similar to one from the other packages ? |
3306f48
to
7907338
Compare
Yes, I will once this PR is complete. The scope of this package is still in the works. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very cool!
i've jotted down some initial thoughts...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General comments after a quick scan:
- There should be room to simplify the creation of AGO query parameters. The logic is complicated in
opendata-ui
because it needs to search both AGO and Hub backend, but it is simpler here. - There should be interface(s) to define the search results in JSONAPI.
b34b790
to
ebe40fb
Compare
i accidentally dismissed @haoliangyu's review, but i think all his feedback was addressed anyway. @pranavkulkarni is this PR ready for another round of code review? |
@jgravois this PR is ready |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true hero's work @pranavkulkarni.
it looks like you haven't done much in the way of properly formatted TSDoc to ensure that folks outside of tugboat will be able to visit our documentation and grok how your first order methods work.
to be fair though, our existing initiative admin methods are every bit as opaque as the stuff you're adding here.
after you deal with my (minor) quips, this seems ready to
packages/search/package.json
Outdated
@@ -0,0 +1,61 @@ | |||
{ | |||
"name": "@esri/hub-search", | |||
"version": "1.11.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, but we released v2.0.0
when you were out on monday. if you update this lerna can help us release v2.1.0
afterward with your new package.
packages/search/package.json
Outdated
"@esri/arcgis-rest-portal": "^2.0.0", | ||
"@esri/arcgis-rest-request": "^2.0.0", | ||
"@esri/arcgis-rest-types": "^2.0.0", | ||
"@esri/hub-common": "^1.11.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"@esri/hub-common": "^2.0.0"
packages/search/package.json
Outdated
"@esri/arcgis-rest-portal": "^2.0.0", | ||
"@esri/arcgis-rest-request": "^2.0.0", | ||
"@esri/arcgis-rest-types": "^2.0.0", | ||
"@esri/hub-common": "^1.11.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"@esri/hub-common": "^2.0.0"
|
||
import { ISearchOptions } from "@esri/arcgis-rest-portal"; | ||
|
||
export interface ISearchParams extends ISearchOptions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not a big fan of conflating Params
and RequestOptions
here, but @dbouwman resisted my attempts to enforce method signatures that dig everything out of a single argument too.
what i guess i'm saying is, 'carry on'.
{}, | ||
customAggsFunctions[customAgg](agoAggregations) | ||
); | ||
facets3 = Object.assign({}, facets3, format(rawCounts)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not sure if it helps with Object.assign()
too but i'm under the impression that tslib
optimizes the destructuring we're doing everywhere else.
const facets3 = { ...format(rawCounts) }
@@ -0,0 +1,21 @@ | |||
import { getProp } from "@esri/hub-common"; | |||
const apiTypes = ["Feature Service", "Map Service", "Image Service"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i feel like you're declaring this in at least two different places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! I'll move it to hub-common
import { getProp } from "@esri/hub-common"; | ||
const apiTypes = ["Feature Service", "Map Service", "Image Service"]; | ||
|
||
export function hasApi(queryFilters: any) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when this library is bundled as a UMD all the methods hang off of an arcgisHub
namespace.
this means that in the browser, filters.hasApi
will collide with aggs.hasApi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting... the UI is not supposed to calls these functions directly. They are being used by agoSearch
and computeItemsFacets
that are exposed by this library and the UI will talk to just those functions. Do I have to still rename them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i didn't see before that you included "hasApi"
in the blacklist. thats definitely good.
even if callers that load the UMD don't call these methods directly though, things will go pear shaped when they're called internally, because i don't think there is a mechanism in place in that build to differentiate them.
you have two options:
- make the names unique, just to be safe.
- test loading the UMD in the browser and calling both methods that lean on
hasApi
internally to confirm that i'm wrong.
can you give this a once over too @tomwayson? |
Thanks @jgravois I will address the points you raised here. I will also beautify comments. |
2342733
to
173606e
Compare
package-lock.json
Outdated
@@ -8116,6 +8116,7 @@ | |||
"color-convert": "^1.9.0" | |||
} | |||
}, | |||
<<<<<<< HEAD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is what Travis is choking on.
besides fixing it manually, you're also welcome to just delete the file and rerun npm install
in the root of the repo to recreate it.
i'll release Hub.js |
Module to search for ArcGIS items and format them for display in ArcGIS Hub
Structure --
Sample of how this will be used in UI code --
Sample of how hub indexer api v3 new search route will use hub.js --