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

Add Import Package page to catalog #1421

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

Add Import Package page to catalog #1421

wants to merge 3 commits into from

Conversation

justinfagnani
Copy link
Collaborator

This adds a /catalog/import GET endpoint for the import page and POST endpoint for the import operation.

The page is extremely basic and just dumps the JSON output of the response to the screen.

Did a few cleanups on route file organization to make things simpler too.

@github-actions
Copy link

A live preview of this PR will be available at the URL below and will update on each commit. =
The build takes ~5-10 minutes, and will 404 until finished.

https://pr1421---site-khswqo4xea-wl.a.run.app/

Warning
Until our Cloud Run project is public, only authorized users can view the above URL.
The easiest way to view it authenticated is to run the following proxy command and visit http://localhost:5453

(gcloud beta run services proxy --project=webcomponents-org-test --region=us-west2 --tag=pr1421 --port=5453 site \
& gcloud beta run services proxy --project=webcomponents-org-test --region=us-west2 --tag=pr1421 --port=6453 catalog)

Copy link
Contributor

@aomarks aomarks left a comment

Choose a reason for hiding this comment

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

Needs a test? Lots of things could go wrong here, e.g. the graphql query is unvalidated I believe?

export class WCOCatalogImportPage extends WCOPage {
private _importTask = new Task(this, {
task: async ([packageName]: [packageName: string | undefined]) => {
console.log('_importTask', packageName);
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove log?

packageName,
}),
});
const result = await response.json();
Copy link
Contributor

Choose a reason for hiding this comment

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

Think we need to check the response code before parsing the json? Since fetch doesn't throw on HTTP errors.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we can let .json() throw and add a error template.

unknown
>
) => {
console.log('context.request.body', context.request.body);
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove log

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.

2 participants