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 catalog-server and catalog-api packages #1323

Merged
merged 18 commits into from
Sep 30, 2022
Merged

Conversation

justinfagnani
Copy link
Collaborator

catalog-api contains the GraphQL schema for the forthcoming GraphQL API to the catalog. It's useful here, before implementing the GraphQL endpoint, to define the schema interfaces that the catalog implementation will be dealing with.

catalog-server is a package that will implement the GraphQL endpoint. This PR add a Catalog class with a couple of basic operations: import package version, and get package version. A Catalog relies on two other interfaces Repository and PackageFiles. Repositoryrepresents the database, and there's currently only one implementation for Firestore.PackageFiles` is the interface for retrieving package metadata and files, usually from the npm registry and a CDN like unpkg.com. There is one implementation for production, and test implementations that load form memory or local files.

Copy link
Collaborator

@rictic rictic left a comment

Choose a reason for hiding this comment

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

A handful of comments / questions from my first pass

"""
The dist tags assigned to this version
"""
distTags: [String!]!
Copy link
Collaborator

Choose a reason for hiding this comment

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

what happens when we notice that a distTag is moved to a new version? we write to both PackageVersions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah. This is what I'm working on right now. We only know the dist-tags when we import a package, and then because we denormalize we have to update all package versions and all custom elements that had a dist-tag change. Since we store the dist-tags, we can calculate if/when we need to do that.

packages/catalog-api/src/lib/schema.graphql Outdated Show resolved Hide resolved
packages/catalog-server/src/lib/npm.ts Show resolved Hide resolved
packages/catalog-server/test.sh Show resolved Hide resolved
Copy link
Collaborator

@rictic rictic left a comment

Choose a reason for hiding this comment

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

A handful of comments / questions from my first pass

@justinfagnani
Copy link
Collaborator Author

Thanks @rictic ! I'll update the comment, and I have another PR I'm about to push that improves the schema types like we were talking about.

Copy link
Collaborator

@rictic rictic left a comment

Choose a reason for hiding this comment

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

Let's configure CI to test with a real firestore instance. I'm confident that tests for important scenarios will get written, so my priority here is just that we have a test environment that's real enough to catch problems before production

The emulator is nice, but I don't trust like that

I don't trust like that

packages/catalog-api/src/lib/schema.graphql Outdated Show resolved Hide resolved
packages/catalog-api/src/lib/schema.graphql Show resolved Hide resolved
@justinfagnani
Copy link
Collaborator Author

@rictic

Let's configure CI to test with a real firestore instance.

I'm ok with this too, but have to figure out how to do it. The nice thing about the emulator is that you start with a fresh database each time it starts. If we test on real Firestore we'll have to figure out how to reliably wipe the database. Also, I have not deployed any version of this code to GCP yet. I'll have to create a new test-only GCP project too.

@justinfagnani
Copy link
Collaborator Author

Regarding real Firestore: there is a bit of work to do here, both on the testing side and on getting the GCP deployments setup for the first time.

I filed #1332 to track the testing setup and code changes required. I'd rather not block PRs that have emulated tests, which in theory should be accurate enough for unit testing - the Firestore docs say so - and work in parallel towards preview and test GCP deployments. @rictic

Add DO_NOT_LAUNCH comments to unimplemented methods.
@rictic
Copy link
Collaborator

rictic commented Sep 23, 2022

If we test on real Firestore we'll have to figure out how to reliably wipe the database.

I don't think that's a good idea, because it would mean multiple tests running at the same time would break each other. Better to lean into the namespaces work, but use random namespaces when in test.

@rictic
Copy link
Collaborator

rictic commented Sep 23, 2022

I'm ok punting the use of a real firestore as long as we have agreement that we should be running all tests in CI with the real firestore before launch.

if we don't have agreement we should talk more :)

@justinfagnani
Copy link
Collaborator Author

If we test on real Firestore we'll have to figure out how to reliably wipe the database.

I don't think that's a good idea, because it would mean multiple tests running at the same time would break each other. Better to lean into the namespaces work, but use random namespaces when in test.

This is exactly what I mean. The clean up function would clean all collections tagged with the namespace used in that test run. The test setup would generate a namespace used by the test and cleanup script. This would be done specifically to isolate test runs and enable parallelism.

I'm ok punting the use of a real firestore as long as we have agreement that we should be running all tests in CI with the real firestore before launch.

if we don't have agreement we should talk more :)

Very much agreement. The namespacing, parallelism, and cleanup work can be developed against both local emulator or the GCP test project, and we can hit either from local dev or CI. As soon as we have the GCP infra ready used with the switch of a flag in a YAML file.

packages/catalog-api/package.json Outdated Show resolved Hide resolved
packages/catalog-api/package.json Outdated Show resolved Hide resolved
packages/catalog-server/src/lib/catalog.ts Outdated Show resolved Hide resolved
packages/catalog-server/src/lib/catalog.ts Outdated Show resolved Hide resolved
packages/catalog-server/tsconfig.json Show resolved Hide resolved
packages/catalog-api/package.json Outdated Show resolved Hide resolved
packages/catalog-api/package.json Outdated Show resolved Hide resolved
packages/catalog-api/src/lib/schema.graphql Show resolved Hide resolved
packages/catalog-server/firestore.rules Show resolved Hide resolved
packages/catalog-server/package.json Outdated Show resolved Hide resolved
packages/catalog-server/tsconfig.json Show resolved Hide resolved
@justinfagnani justinfagnani merged commit 3dfcd39 into main Sep 30, 2022
@justinfagnani justinfagnani deleted the catalog-server branch September 30, 2022 20:13
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.

3 participants