feat(gteam-internal): add gteam-internal subpath export, migrate tests to Vitest - #5
Conversation
…s to Vitest Merges in a private package's safePushData as a new ./gteam-internal export (wraps Actor.pushData/Dataset.pushData directly, unlike the top-level pushFn-injection safePushData). Consolidates the whole test suite onto Vitest since the new module's tests need module mocking that node:test can't do cleanly, and running two test frameworks side by side wasn't worth the upkeep.
Keeps the private-package migration context out of the public README; the top-level doc now just links to src/gteam-internal/README.md.
JuanGalilea
left a comment
There was a problem hiding this comment.
how exactly is this a safePushData?
The only thing i see here is it turning dataset validation errors into NonRetryableError but no healing or retrying with healed inputs like the other one.
Also, the reasoning behind merging them is kinda questionable. Wouldn't this be a prime target for some Partial application? all pushData's have the exact same signature, and safePushData conveniently takes it as an argument.
Couldn't it be done as partial application on your consumer?
type DatasetParameters = Parameters<typeof safePushData<DatasetItem, PushDataOutput>>
export const pushDataToDatasetX = (data: DatasetParameters[1], options: DatasetParameters[2]) => safePushData(Dataset.pushData, data, options);And then you use pushDataToDatasetX whenever you want to push there? You also get type safety on the call location, which you don't get on the current solution.
All of this also gets rid of the peerDependencies, which is a plus in my books.
If the intent is only to turn the data validation errors into some other one, this could be done with some Higher order Function or something, without relying on this library to do the pushData or anything
Resolves conflicts from master's safePushData -> pushDataWithSchemaRepair rename (#6): our vitest-ported test file and CHANGELOG unreleased section adjusted to match. Also drops gteam-internal from the README's project structure diagram (its docs already moved to src/gteam-internal/README.md).
JuanGalilea
left a comment
There was a problem hiding this comment.
after some nice discussion with @yaroslav-tykhovetskyi-apify we got to it kinda doesn't matter as long as its in a google-teams namespace.
Although i still dont like the purpose of the function 😅
| "peerDependencies": { | ||
| "@apify/log": "^2.5.44" | ||
| "@apify/log": "^2.5.44", | ||
| "@crawlee/core": ">=3.16.0", |
There was a problem hiding this comment.
these should be tilde, not >=
@crawlee/core, apify, and apify-client all have 4.0.0 betas published; capping with ^ instead of >= avoids silently resolving into a breaking major bump. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Caps to patch-level updates within the minor version that introduced the APIs the code depends on, instead of allowing any minor bump. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
radimkvet
left a comment
There was a problem hiding this comment.
well, too late for those, but maybe they can be implemented in different PR.
| * const { chargeableWithinLimit } = await safePushData(item, { eventName: 'result-scraped' }); | ||
| * ``` | ||
| */ | ||
| export async function safePushData<T extends object>( |
There was a problem hiding this comment.
And we can also do it here:
function safePushData<T extends object>(
data: T | T[],
options: {alias: never, eventName: string}
)This way, we will explicitly say "we do not support those two together"
| * await safePushData(report, { alias: 'competitorAnalysis' }); | ||
| * ``` | ||
| */ | ||
| export async function safePushData<T extends object>(data: T | T[], options?: { alias: string }): Promise<void>; |
There was a problem hiding this comment.
Since we do not support the two options together, we can be more explicit in the type, so the type system tells us we are doing something wrong:
function safePushData<T extends object>(
data: T | T[],
options: {alias: string, eventName: never}
)
Why?
A private internal package's safePushData wrapper is being merged into this repo as a subpath export, so it can be maintained alongside the existing top-level safePushData instead of as a separate package. Its test suite needs to mock the Apify SDK, which node:test can't do cleanly, so this was also a chance to stop running two different test frameworks side by side.
What?
Adds a new
./gteam-internalexport backed bysrc/gteam-internal/. Unlike the top-levelsafePushData, this one callsActor.pushData/Dataset.pushDatadirectly instead of taking apushFn, soscripts/check-pushdata.mjsnow excludes that subfolder from its guard.apify,apify-client, and@crawlee/coreare added as optional peer dependencies, needed only by consumers of this new subpath.The whole test suite (top-level and new) now runs on Vitest. The old
node:testsuite was ported over with an equivalent assertion for assertion, and thetest/cinpm scripts were simplified down to a singlenpm testrun.Further notes
coverage/is now gitignored and excluded from Prettier, since Vitest's coverage reporter didn't exist as an output before this change.