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

fix [GreasyFork] 404 bug #9632

Merged
merged 1 commit into from Oct 16, 2023
Merged

fix [GreasyFork] 404 bug #9632

merged 1 commit into from Oct 16, 2023

Conversation

hymbz
Copy link
Contributor

@hymbz hymbz commented Oct 2, 2023

refs #9611

@github-actions
Copy link
Contributor

github-actions bot commented Oct 2, 2023

Messages
📖 ✨ Thanks for your contribution to Shields, @hymbz!

Generated by 🚫 dangerJS against 7ca212c

@chris48s
Copy link
Member

chris48s commented Oct 9, 2023

Can you explain the relationship between greasyfork and sleazyfork for someone who doesn't use either?

It is clear they are closely related services, but my instinct is that it would make more sense to add sleazyfork as its own service, rather than treating it as a fallback for greasyfork.

@chris48s chris48s added the service-badge Accepted and actionable changes, features, and bugs label Oct 9, 2023
@hymbz
Copy link
Contributor Author

hymbz commented Oct 10, 2023

See https://greasyfork.org/en/forum/discussion/5456/adult-content for reasons. Basically, if a script works on an "adult" sites (even if it's one such site out of a thousand), it cannot be seen on GreasyFork without being logged in.

Sleazyfork is another inlet to Greasyfork, where adult scripts can be displayed without logging in and only adult scripts are visible. Before, this type of script could still retrieve data through the API of Greasyfork, but now it is no longer possible.

@chris48s
Copy link
Member

Ok. Thanks for that context. This does seem reasonable.

Two things:

  1. Instead of just catching any error, can we explicitly handle NotFound - if there is some other error, there's probably no point making another network call
  2. I don't think we need to add one for every single greasyfork badge (downloads, license, rating and version) but can we add at least one service test that follows the sleazyfork path somewhere.

Thanks

@hymbz
Copy link
Contributor Author

hymbz commented Oct 13, 2023

You're right, I have made the necessary changes to the code.

url: `https://greasyfork.org/scripts/${scriptId}.json`,
})
} catch (e) {
if (e.message !== 'Not Found') throw e
Copy link
Member

Choose a reason for hiding this comment

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

Could we check for the exception type using instanceof so it will still work if we customise the not found error.

e.g:

import { NotFound } from '../index.js'

...

if (!(e instanceof NotFound)) throw e

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay

@github-actions
Copy link
Contributor

🚀 Updated review app: https://pr-9632-badges-shields.fly.dev

Copy link
Member

@chris48s chris48s left a comment

Choose a reason for hiding this comment

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

nice - thanks

@chris48s chris48s added this pull request to the merge queue Oct 16, 2023
Merged via the queue into badges:master with commit 6f2c0a2 Oct 16, 2023
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge Accepted and actionable changes, features, and bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants