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.

Sign up for GitHub

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: do validation for params off constructor so it can throw before starting #1984

Merged
merged 4 commits into from
Nov 18, 2024
Merged

fix: do validation for params off constructor so it can throw before starting #1984

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b394d04
fix: do validation for params off constructor so it can throw before …
KyleAMathews Nov 16, 2024
22c1dcf
Remove only
KyleAMathews Nov 18, 2024
77a68ba
style
KyleAMathews Nov 18, 2024
4b36367
add config.d.ts
KyleAMathews Nov 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 28 additions & 1 deletion packages/typescript-client/src/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,19 @@

type Replica = `full` | `default`

type ReservedParamKeys =
| typeof DATABASE_ID_QUERY_PARAM
| typeof COLUMNS_QUERY_PARAM
| typeof LIVE_CACHE_BUSTER_QUERY_PARAM
| typeof SHAPE_HANDLE_QUERY_PARAM
| typeof LIVE_QUERY_PARAM
| typeof OFFSET_QUERY_PARAM
| typeof TABLE_QUERY_PARAM
| typeof WHERE_QUERY_PARAM
| typeof REPLICA_PARAM

type ParamsRecord = Omit<Record<string, string>, ReservedParamKeys>

/**
* Options for constructing a ShapeStream.
*/
Expand Down Expand Up @@ -113,8 +126,10 @@
/**
* Additional request parameters to attach to the URL.
* These will be merged with Electric's standard parameters.
* Note: You cannot use Electric's reserved parameter names
* (table, where, columns, offset, handle, live, cursor, database_id, replica).
*/
params?: Record<string, string>
params?: ParamsRecord

/**
* Automatically fetch updates to the Shape. If you just want to sync the current
Expand Down Expand Up @@ -517,5 +532,17 @@
`shapeHandle is required if this isn't an initial fetch (i.e. offset > -1)`
)
}

// Check for reserved parameter names
if (options.params) {
const reservedParams = Object.keys(options.params).filter(

Check failure on line 538 in packages/typescript-client/src/client.ts

View workflow job for this annotation

GitHub Actions / Check packages/typescript-client 

Replace `⏎······(key)·=>` with `(key)·=>⏎·····`
(key) => RESERVED_PARAMS.has(key)
)
if (reservedParams.length > 0) {
throw new Error(
`Cannot use reserved Electric parameter names in custom params: ${reservedParams.join(`, `)}`
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this could be simpler?

for (const param of RESERVED_PARAMS) {
  if (param in options.params) {
    throw new Error(`Cannot use reserved Electric parameter name in custom params: ${param}`);
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The existing test checks all parameters before throwing an error — so the error message is more comprehensive than if you throw immediately.

}
return
}
3 changes: 3 additions & 0 deletions packages/typescript-client/test/__snapshots__/client.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html

exports[`Shape > should throw on a reserved parameter 1`] = `[Error: Cannot use reserved Electric parameter names in custom params: database_id]`;
14 changes: 14 additions & 0 deletions packages/typescript-client/test/client.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,20 @@ describe(`Shape`, () => {
expect(shape.lastSynced()).toBeLessThanOrEqual(Date.now() - start)
})

it.only(`should throw on a reserved parameter`, async () => {
console.log(`hi`)
expect(() => {
const shapeStream = new ShapeStream({
url: `${BASE_URL}/v1/shape`,
table: `foo`,
params: {
database_id: `foo`,
},
})
new Shape(shapeStream)
}).toThrowErrorMatchingSnapshot()
})

it(`should notify with the initial value`, async ({
issuesTableUrl,
insertIssues,
Expand Down
Loading