feat(hub-discussions): adds Discussions package to Hub.js #479
Conversation
Codecov Report
@@ Coverage Diff @@
## master #479 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 428 438 +10
Lines 6001 6242 +241
Branches 946 993 +47
==========================================
+ Hits 6001 6242 +241
Continue to review full report at Codecov.
|
|
✋Let me know when it's appropriate to provide some questions or comments on this PR. |
@ajturner let em fly |
There was a problem hiding this comment.
Thanks for being open to feedback. I've added some notes based discussions with @mikeringrose & @dbouwman for consistency in Hub.js, and some other recommendations considering longer-term maintenance.
| * @param {IRequestOptions} options | ||
| * @return {*} {Promise<string>} | ||
| */ | ||
| export function authenticateRequest(options: IRequestOptions): Promise<string> { |
There was a problem hiding this comment.
These methods seem like they would be used across this library. Are there Discussion-specific reasons it's in this module or should they be in a core utils?
If possible, that would reduce redundancy, discrepancies, and maintenance.
There was a problem hiding this comment.
TBH I didn't give much thought about the location of this method. I assumed this was the only place that needed to use platform authentication to make an off-platform request. Is that not the case?
There was a problem hiding this comment.
We already have:
hub.js/packages/common/src/request.ts
Line 32 in f35b1a0
and a few places where we append the Authorization header, although those seem to be missing the "Bearer " prefix to the token. I have some vague recollection of a discussion between John Gravois and @rgwozdz regarding that at some point.
I think that the request and auth logic should be centralized in hub-common.
packages/discussions/src/posts.ts
Outdated
|
|
||
| /** | ||
| * update post | ||
| * NOTE: this method only updates a post's title and/or body |
There was a problem hiding this comment.
This is a good limit to note.
what are your plans for adding update support to geometry or other post attributes?
There was a problem hiding this comment.
There are separate methods for updating a post's status (only allowed by moderators) and its channel/sharing. This method could support geometry and discussion/target in just a few LOC; it would be beneficial to discuss the business logic about which attributes are updatable and by whom.
packages/discussions/src/posts.ts
Outdated
| */ | ||
| export function searchPosts( | ||
| options: IQueryPostsOptions | IQueryChannelPostsOptions | ||
| ): Promise<INestPagination<IPostDTO>> { |
There was a problem hiding this comment.
These names are bleeding the implementation into hub.js. I'd change IPostDTO to just IPost and INestPagination shouldn't be. We really need to be consistent in how we do things so if at all possible pagination should be uniform across hub.js
There was a problem hiding this comment.
I agree w/ @mikeringrose on this... from what I can tell, we should be able to drop DTO from everything that has it... Using Rename Symbol in vscode, this should be pretty easy.
There was a problem hiding this comment.
agreed, *DTO has been removed. For pagination, what's the gold standard? I've found IPagedResponse and IPagingParams, but I've also seen other interfaces like https://github.com/Esri/hub.js/blob/master/packages/search/src/types/common.ts#L38
There was a problem hiding this comment.
Also, would it be appropriate for me to leverage SortDirection from hub-search in my query DTO's?
There was a problem hiding this comment.
I think so @cpgruber
packages/discussions/src/posts.ts
Outdated
| if (options.params.hasOwnProperty("channelId")) { | ||
| const { | ||
| params: { channelId } | ||
| } = options as IQueryChannelPostsOptions; | ||
| url = `/channels/${channelId}` + url; |
There was a problem hiding this comment.
Why just one method? Why not a method searchPost and searchChannel? It allows code to be more explicit and you remove the if condition and you force validation by parameters.
There was a problem hiding this comment.
So this method is searching posts; IQueryChannelPostsOptions scopes the search to a single channel, while IQueryPostsOptions can search many channels. Does that change your opinion?
Otherwise, I thought having two methods, searchPosts and searchChannelPosts, to be redundant and consolidated to a single method for the convenience of dev users.
| THUMBS_UP = "thumbs_up", | ||
| THUMBS_DOWN = "thumbs_down", | ||
| THINKING = "thinking", | ||
| HEART = "heart", | ||
| ONE_HUNDRED = "one_hundred", | ||
| SAD = "sad", | ||
| LAUGH = "laugh", | ||
| SURPRISED = "surprised" |
There was a problem hiding this comment.
since these are comments, just use the UTF8 emoji character 👍, 👎, etc.
There was a problem hiding this comment.
I'm sure it would work just fine, but since these values are ultimately persisted in the database I figure using human-readable string versions would eliminate any potential for things to go wrong if and when we choose to optimize by migrating these string values to numeric
There was a problem hiding this comment.
The UTF8 emoji character would be numeric. For example, thumbs-up is U+1F44D.
cpgruber
changed the title
Adds
discussionspackage to monorepo, which will serve as the interface layer in front of the Discussions API and ultimately replace hub-annotations (TBDeprecated)