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 Account API support and container management #556

Merged
merged 6 commits into from
Jan 9, 2025
Merged

Conversation

ecooper
Copy link
Contributor

@ecooper ecooper commented Jan 9, 2025

Problem

While working on #554 to support v2 Account API endpoints, a few sharp edges emerged:

  • We had no clean way to support v1 vs v2 endpoints
  • Account URL was not always read from argv
  • It was impossible to modify the DI container outside of run, which makes it impossible to test library code directly
  • Testing account endpoints required stubbing and testing the fetch layer (vs just stubbing the data you expect)
  • Validation and behavior of account endpoints was mixed between account.mjs and fauna-account-client.mjs.

Solution

While this could have been at least 2 separate PRs (container changes first, then api client changes), I didn't discover this problem until testing the API changes. If reviewing this is too bothersome, I can break them apart.

The major changes include:

  • Refactor FaunaAccountClient class into set of api methods exported as accountAPI: account.mjs and fauna-account-client.mjs have effectively been merged into account-api.mjs. This module contains all the API endpoints we currently support in the CLI as well as helpers to handle Account API responses.
  • Support v1/v2 endpoints: Requests can now be made to the Account API v1 and v2 endpoints. Errors from v2 endpoints are supported. Endpoint implementations can manage this by passing version to the fetch helper in the module.
  • The container singleton is now in ./config/container.mjs: In order to test library code directly, we need to be able to set the container outside of run contexts. By moving the container singleton into a different module with a setter, setContainer, we can easily manage dependency resolution in tests outside of run executions.

Places to pay attention:

  • ./config/container.mjs
  • ./lib/account-api.mjs
  • ./cli.mjs
  • ./test/mocha-root-hooks.mjs

Everything else is mostly import changes or test changes.

Result

You can now call v1 and v2 endpoints. Additionally, the account API interface has been simplified to be:

import { container } from './config/container.mjs'

// <..snip>
const { listDatabases } = container.resolve('accountAPI')
await listDatabases({ path: 'us-std/demo', pageSize: 10});

Testing

All impacted code paths have either a) new tests or b) updated tests.

src/cli.mjs Outdated
const argvInput = _argvInput;
const logger = container.resolve("logger");
const parseYargs = container.resolve("parseYargs");
const logger = _container.resolve("logger");
Copy link
Collaborator

Choose a reason for hiding this comment

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

rename to container? the declaration these used to conflict with no longer exists

Comment on lines +22 to +28
/**
* Standardizes the region of a database path.
*
* @param {string | undefined} databasePath - The database path to standardize.
* @returns {string | undefined} The standardized database path.
* @throws {TypeError} If the database path is not a string.
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

if this also confirmed the rg was valid, it could save us a lot of distributed correctness checks. thoughts?

@ecooper ecooper merged commit 334ed9f into main Jan 9, 2025
4 checks passed
@ecooper ecooper deleted the account-api-v2 branch January 9, 2025 17:44
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.

4 participants