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

Refactor to dub TypeScript SDK #13449

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

steven-tey
Copy link
Contributor

@steven-tey steven-tey commented Jul 12, 2024

Description

Simplify Dub Raycast Extension by using dub TypeScript SDK.

Also added support for shortening links with custom domains:

CleanShot 2024-07-15 at 08 20 36

Checklist

Simplify Dub Raycast Extension by using `dub` TypeScript SDK
@raycastbot
Copy link
Collaborator

raycastbot commented Jul 12, 2024

Thank you for your contribution! 🎉

🔔 @quuu @jfkisafk @steven-tey you might want to have a look.

You can use this guide to learn how to check out the Pull Request locally in order to test it.

We're currently experiencing high demand and limited capacity. As a result, extension reviews might take longer than usual to get the initial review. Please expect an initial review within 5-10 business days.

@raycastbot raycastbot added extension fix / improvement Label for PRs with extension's fix improvements extension: dub-link-shortener Issues related to the dub-link-shortener extension labels Jul 12, 2024
@jfkisafk
Copy link
Contributor

Hey @steven-tey , I tried to use the sdk too, but got the same error before! So I stuck with axios for now.

@steven-tey
Copy link
Contributor Author

@peduarte @thomaslombart any ideas here? 👀

Copy link
Contributor

@jfkisafk jfkisafk left a comment

Choose a reason for hiding this comment

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

We can address these while we try to resolve that Header issue. After that, we might need to see if the DUB error(s) are being properly propagated to the user.

extensions/dub-link-shortener/src/search-links.tsx Outdated Show resolved Hide resolved
extensions/dub-link-shortener/src/search-links.tsx Outdated Show resolved Hide resolved
extensions/dub-link-shortener/package.json Outdated Show resolved Hide resolved
extensions/dub-link-shortener/CHANGELOG.md Outdated Show resolved Hide resolved
steven-tey and others added 3 commits July 14, 2024 18:46
Co-authored-by: stelo <42366677+jfkisafk@users.noreply.github.com>
Co-authored-by: stelo <42366677+jfkisafk@users.noreply.github.com>
@steven-tey
Copy link
Contributor Author

@jfkisafk thank you so much! Addressed all your feedback 🙏

@jfkisafk
Copy link
Contributor

jfkisafk commented Jul 15, 2024

Hey @steven-tey this is not a raycast issue, b/c the error is happening during the getAllShortLinks call:

ReferenceError: Headers is not defined
    at Links.list (/Users/stelo/.config/raycast/extensions/dub-link-shortener/search-links.js:22338:26)
    at getAllShortLinks (/Users/stelo/.config/raycast/extensions/dub-link-shortener/search-links.js:23970:20)
    at /Users/stelo/.config/raycast/extensions/dub-link-shortener/search-links.js:216:77
    at /Users/stelo/.config/raycast/extensions/dub-link-shortener/search-links.js:339:11
    at Cr (/Applications/Raycast.app/Contents/Resources/RaycastNodeExtensions_RaycastNodeExtensions.bundle/Contents/Resources/api/node_modules/react-reconciler/react-reconciler.production.min.js:7:20793)
    at Bn (/Applications/Raycast.app/Contents/Resources/RaycastNodeExtensions_RaycastNodeExtensions.bundle/Contents/Resources/api/node_modules/react-reconciler/react-reconciler.production.min.js:7:39556)
...

In the generated file, the source code is copied from the dub TS SDK, and the Header is not found here:

https://github.com/dubinc/dub-ts/blob/07f40571e93bc17f0fe0000dc14f625cf18c34cc/src/sdk/links.ts#L81-L83

I think the code that Speakeasy is generating is not adding the import/dependency for Headers correctly.

@steven-tey
Copy link
Contributor Author

@jfkisafk this should be the Headers Web API which doesn't need to be imported/defined. Is this not supported in Raycast extensions?

@jfkisafk
Copy link
Contributor

It is strange, I can see that the import is not necessary (my IDEs are also indicating that). Not sure why in the generated .js file this is causing an issue @steven-tey . Let me investigate more.

Or easier debugging option would be to just use new Header() in the src .ts code and see if that throws an error.

@jfkisafk
Copy link
Contributor

jfkisafk commented Jul 15, 2024

Hey @steven-tey the problem is exactly this: https://stackoverflow.com/a/65355673

I tested it by just placing:

new Headers({
  "Content-Type": "application/json",
})

in the codepath without the node-fetch import and it failed with Headers not found error at that location. But when I added import {Headers} from "node-fetch", it succeeded. So I think an amendment in the SDK or Speakeasy generation is required to gracefully handle this.

@steven-tey
Copy link
Contributor Author

steven-tey commented Jul 15, 2024

@jfkisafk just added a polyfill patch thanks to @disintegrator that fully fixes this! 🤩

CleanShot.2024-07-15.at.08.21.45.mp4

@steven-tey steven-tey marked this pull request as ready for review July 15, 2024 15:21
@@ -0,0 +1,81 @@
diff --git a/extensions/dub-link-shortener/package-lock.json b/extensions/dub-link-shortener/package-lock.json
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this file necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

@steven-tey I had the same question, it looked like a git patch that was already applied. Shall we clean this up?

Copy link
Contributor

@mil3na mil3na left a comment

Choose a reason for hiding this comment

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

Other than this .patch file, things look okay on my side. Ready to merge after I get some answers :)

@mil3na mil3na self-assigned this Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension: dub-link-shortener Issues related to the dub-link-shortener extension extension fix / improvement Label for PRs with extension's fix improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants