PipedreamHQ / pipedream

Connect APIs, remarkably fast. Free for developers.
https://pipedream.com
Other
8.32k stars 5.27k forks source link

[ACTION] Clubhouse.io (now Shortcut) #1383

Closed sergioeliot2039 closed 2 years ago

sergioeliot2039 commented 2 years ago

PR for Clubhouse actions Create Story Search Stories

vercel[bot] commented 2 years ago

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

pipedream-docs-redirect-do-not-edit – ./docs

🔍 Inspect: https://vercel.com/pipedreamers/pipedream-docs-redirect-do-not-edit/9YrZKUkvSS3shH2tFVznm6grohKg
✅ Preview: https://pipedream-docs-redirect-do-not-edit-git-for-41f22e-pipedreamers.vercel.app

pipedream-docs – ./docs

🔍 Inspect: https://vercel.com/pipedreamers/pipedream-docs/CN7PVurxgJzoQBvy6rTY1sMV7L4M
✅ Preview: https://pipedream-docs-git-fork-sergioeliot2039-clu-db59d8-pipedreamers.vercel.app

sergioeliot2039 commented 2 years ago

@compwright when i started developing components, the criteria to use a library was:

  1. be an official library
  2. have thousands of weekly downloads
  3. be mantained oftenly.

in the case for if you check stats in npm for clubhouse-lib the lib is official and has ~3k weekly download, but last update was over 5 months ago. that's why I didn't use it. another parameter I use is that usually PD team will use the library in Legacy actions' "Run node.js code with CLubhouse", and they are using the API directly.

dylburger commented 2 years ago

@sergioeliot2039 I think we can be a little loose on #2 and #3 if it's an official library. It looks like the maintainers are pushing new releases every so often. It's probably the case that the API hasn't changed in the last few months. This is a tough call but in this specific case I'd use the npm package. Clubhouse isn't the largest app, and that's part of the reason why I think the number of downloads is low.

sergioeliot2039 commented 2 years ago

@compwright clubhouseio actions ready for review. options for groupID not implemented -- library returning wrong id.

compwright commented 2 years ago

@sergioeliot2039 I assume you are still working on this, there are still a few review items that need to be addressed, and an eslint issue or two.

sergioeliot2039 commented 2 years ago

@sergioeliot2039 I assume you are still working on this, there are still a few review items that need to be addressed, and an eslint issue or two.

that's right i have the members() dynamicly populating options, and maybe someothers, working on this now. yeah eslint will check everything before calling it done.

sergioeliot2039 commented 2 years ago

@compwright i'm done with changes as per review, turning over to you. thanks!

compwright commented 2 years ago

@sergioeliot2039 I'm still seeing a number of unresolved/unaddressed changes requested from my last review. Please take another look.

compwright commented 2 years ago

@sergioeliot2039 @dylburger Clubhouse.io is renaming to Shortcut in two weeks. We should go ahead and include that here. I assume there will have to be a rename at the app auth level, and then the file paths and app name will need to be renamed here. Also update all descriptions wherever they might reference clubhouse.io.

https://clubhouse.io/blog/clubhouse-changing-our-name-to-shortcut

sergioeliot2039 commented 2 years ago

Yes previously I reached out to them on that issue in the Clubhouse community and this is what they had to say:

Nicolas Charpentier:clubhouse: Aug 10th at 12:24 PM Hello @Sergio Rodriguez! :wave: We’re planning on renaming clubhouse-lib . The “breaking changes” will be: A new name for the library published on npm. The API_BASE_URL will be updated to point to something that isn’t clubhouse. (Should be seamless). So, if you want to use clubhouse-lib right now, you’ll have to do swap it with another package and then do some find and replace but it shouldn’t be a headache. How does that sound?

I'm about to look if there's been any change on their end already and make changes accordingly.

sergioeliot2039 commented 2 years ago

@compwright i have changes ready for a review, there are some comments in the requested changes that need your input please. as of the app renaming to shortcut, the only change so far doesn't affect the implementation, that is the header name for sending the auth token in the request, because that is wrapped inside the sdk. on sep. 14th they are set to change the url and perhaps the sdk will be renamed too by then. would you mind taking a look with this date in mind? thanks!

sergioeliot2039 commented 2 years ago

@compwright @dylburger i've pushed changes to Clubhouse as per the lastest review comments. and it is ready for a new review.

sergioeliot2039 commented 2 years ago

i delayed a bit these changes because of the changes last months after Clubhouse was renamed to Shortcut. The clubhouse-lib had non breaking changes which were implemented. they also shipped a new library -->https://github.com/useshortcut/shortcut-client-js , renamed with breaking changes. i tried to implement but i found there are bugs there. create story worked using the new library however, i noticed pagination on search stories in the new sdk is not working properly. i tried using the fetchNext() method for pagination in previous SDK and it doesn't exist anymore https://pipedream.com/@sergio/shortcut-search-stories-with-new-sdk-doesn-t-have-a-fetchnext-method-for-pagination-p_vQCzB3M/edit using the search method, https://github.com/useshortcut/shortcut-client-js/blob/0d0151b/src/generated/data-contracts.ts#L3703 which accepts a "next" parameter for pagination, doesn't work, it returns the same set of data regardless of the next token being set. https://pipedream.com/@sergio/shortcut-search-stories-with-new-sdk-using-search-method-p_wOCzLnm/edit using the searchstories method, https://github.com/useshortcut/shortcut-client-js/blob/0d0151b/src/generated/data-contracts.ts#L3720 it doesn't accept a next parameter, so including in the parameters results in 422 Unprocessable entity https://pipedream.com/@sergio/shortcut-search-stories-with-new-sdk-searchstories-method-p_pWCQ1Oz/edit

sergioeliot2039 commented 2 years ago

so i think is best to ship this components with "clubhouse-lib" and maybe update to the newer usdk "@useshortcut/client" in a new wave of changes for shortcut/clubhhouse components, expecting the new SDK has been improved. @dylburger @compwright

dylburger commented 2 years ago

@sergioeliot2039 are they planning to completely remove clubhouse-lib from npm, or will that package remain and just not receive updates?

Did you reach out to them re: the pagination issue to see if they have a suggested workaround?

compwright commented 2 years ago

@sergioeliot2039 It looks like you can pass arbitrary request params in the second argument of searchStories() - see https://github.com/useshortcut/shortcut-client-js/blob/0d0151bf2800ba85dbe8b1cab657a539d16f1fa2/src/generated/Api.ts#L1443

Instead of this:

const { ShortcutClient } = require('@useshortcut/client');
const shortcut = new ShortcutClient(auths.shortcut.api_key);
return await shortcut.search({ query: "is:story CRE", page_size: 2, next: "325a9d1c8c053d0096a706ea111066278bcaf0d2~1" });

Try this:

const { ShortcutClient } = require('@useshortcut/client');
const shortcut = new ShortcutClient(auths.shortcut.api_key);
return await shortcut.search({ query: "is:story CRE" }, { page_size: 2, next: "325a9d1c8c053d0096a706ea111066278bcaf0d2~1" });
sergioeliot2039 commented 2 years ago

@compwright I had seen before that documentation and tried as you suggested but used as you point out and the page_size param isnot honored, records are still repeated. i am about to reach out to shortcut support for workarounds.

sergioeliot2039 commented 2 years ago

@jcortes this ready for a new review.

dannyroosevelt commented 2 years ago

The Create Story action functionally works, but it returns the below console.log() statement: CIRCULAR_RETURN_VALUEReturn value contains [Circular] reference(s) that were filtered out.

dannyroosevelt commented 2 years ago

@sergioeliot2039 I pushed a few changes and created a new PR from the pipedream repo, so it's in sync w/ master

vercel[bot] commented 2 years ago

@sergioeliot2039 is attempting to deploy a commit to the Pipedreamers Team on Vercel.

A member of the Team first needs to authorize it.

sergioeliot2039 commented 2 years ago
@dannyroosevelt

this has been corrected. i have move its card to ready for qa. but let me know if it should be in ready for review instead.

dannyroosevelt commented 2 years ago

Looks good, @sergioeliot2039 ! I see there are some Lint errors you'll need to address before merging.

sergioeliot2039 commented 2 years ago

Great, @dylburger . I fixed lint errors. Merging blocked because of Vercel - pipedream-docs. Your turn (I guess)!