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

Search ecosystem split #53

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

Search ecosystem split #53

wants to merge 13 commits into from

Conversation

karamba228
Copy link

Weird behavior. the search seems to prioritize Pypi no matter what.
For example, I am using pandas to test pypi and conda packages:

If pypi is selected and I search for "Pandas" the results show up as expected. But if I select coda for the ecosystem and search for "Pandas" the same output is generated.
Screenshot 2025-01-14 154945
Screenshot 2025-01-14 155005

Where the differences start to appear is if I select pypi and search for conda/pandas, pypi results are prioritized. likewise, if I search for conda/pandas with conda ecosystem selected, conda results show up at the top of the list.
Screenshot 2025-01-14 155024
Screenshot 2025-01-14 155221

Copy link

vercel bot commented Jan 14, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
score-site ✅ Ready (Inspect) Visit Preview 2 resolved Feb 7, 2025 7:55pm

@karamba228 karamba228 requested a review from srossross January 14, 2025 21:54
@karamba228 karamba228 linked an issue Jan 14, 2025 that may be closed by this pull request
Copy link
Contributor

@srossross srossross left a comment

Choose a reason for hiding this comment

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

thanks for the update @karamba228 - I've added a few more comments

Comment on lines 1 to 2
// import { maturityColor, maturityIconPath } from "@/components/Maturity";
// import { riskColor, riskIconPath } from "@/components/Risk";
Copy link
Contributor

Choose a reason for hiding this comment

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

remove comments

@@ -4,9 +4,10 @@ import { NextRequest } from "next/server";
export async function GET(request: NextRequest) {
const searchParams = request.nextUrl.searchParams;
const query = searchParams.get("query");
const ecosystem = searchParams.get("ecosystem") || "";
Copy link
Contributor

Choose a reason for hiding this comment

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

this will fail with "" will it not?

Copy link
Author

Choose a reason for hiding this comment

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

I don't think we are using this file anymore. It looks to be handling package search within the API

} catch (error) {
const errorMessage =
error instanceof Error ? error.message : "An unknown error occurred.";
toast.error(`Error fetching data from Anaconda.org: ${errorMessage}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

have you tested this toast? can you send a screenshot? it looks like it is running on the server

Copy link
Author

Choose a reason for hiding this comment

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

You are right, toast does not work server side, I tried propagating the error up to the client side page but the error does not propagate with debounce: lodash/lodash#4449. I looked into the suggested solution and tried implementing that but the error would not propagate: lodash/lodash#4815 (comment)

const errorMessage =
error instanceof Error ? error.message : "An unknown error occurred.";
toast.error(`Error fetching data from npm registry: ${errorMessage}`);
throw new Error(`Error fetching data from npm registry: ${errorMessage}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you throwing a new error here?

Suggested change
throw new Error(`Error fetching data from npm registry: ${errorMessage}`);
throw error;

@srossross
Copy link
Contributor

how is this coming along?

@srossross
Copy link
Contributor

@karamba228 ?

@karamba228
Copy link
Author

@srossross I am blocked with debounce not allowing the propagation of the error to the client side component for the toast to work properly. Retrieving packages from each respective api should work now. I mentioned the block in the previous comment with references to issues I have researched #53 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Search should use underlying repo
2 participants