Switch from yarn to pnpm package manager
#1428
Conversation
|
WalkthroughAdd a pnpm workspace, migrate CI/workflows/docs/manifests from Yarn→pnpm, add a reusable composite GitHub Action to set up Node and run pnpm install, convert many package deps to Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (35)
💤 Files with no reviewable changes (3)
🚧 Files skipped from review as they are similar to previous changes (15)
🧰 Additional context used🪛 LanguageToolCONTRIBUTING.md[grammar] ~164-~164: Use a hyphen to join words. (QB_NEW_EN_HYPHEN) [grammar] ~164-~164: Ensure spelling is correct (QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1) 🪛 markdownlint-cli2 (0.18.1)playgrounds/react/README.md9-9: Fenced code blocks should have a language specified (MD040, fenced-code-language) 14-14: Fenced code blocks should have a language specified (MD040, fenced-code-language) packages/autocomplete-client/README.md57-57: Fenced code blocks should have a language specified (MD040, fenced-code-language) playgrounds/autocomplete/README.md9-9: Fenced code blocks should have a language specified (MD040, fenced-code-language) 14-14: Fenced code blocks should have a language specified (MD040, fenced-code-language) playgrounds/javascript/README.md9-9: Fenced code blocks should have a language specified (MD040, fenced-code-language) 14-14: Fenced code blocks should have a language specified (MD040, fenced-code-language) 🔇 Additional comments (16)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](http://www.gproxx.com/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/instant-meilisearch/README.md (1)
138-141: Avoid publishing realistic-looking API keys in README examples; use a placeholder + explicit warning. This reduces accidental leakage / copy-paste of privileged credentials.- 'a63da4928426f12639e19d62886f621130f3fa9ff3c7534c5d179f0f51c4f303' // API key + 'MEILISEARCH_SEARCH_API_KEY' // API key (use a search-only key; never expose admin/master keys)Also applies to: 168-171
playgrounds/autocomplete/README.md (1)
1-1: README title likely wrong for this playground (# React Playground). Consider renaming to match “Autocomplete Playground” to avoid confusion.packages/autocomplete-client/README.md (1)
91-95: Remove/replace the hard-coded API key in docs (treat as leaked secret).
Even if it’s “just an example”, it’s a real-looking key and will get scraped/copy-pasted. Use a placeholder and call out “search-only key”.- apiKey: 'a63da4928426f12639e19d62886f621130f3fa9ff3c7534c5d179f0f51c4f303' // API key + apiKey: '<YOUR_SEARCH_API_KEY>' // Search-only key (never use master/admin keys in the browser)playgrounds/local-react/package.json (1)
6-12: Document dev server dependency for local E2E testing.The E2E tests reference
baseUrl: http://localhost:5174(fromcypress.env.json), which requires the Vite dev server to be running. While CI handles this viacypress-io/github-action'sstart: pnpm playground:local-react, local developers runningpnpm test:e2ewill encounter flaky failures if the dev server is not already running. Add a note to CONTRIBUTING.md clarifying that developers must runpnpm devbefore executing E2E tests, or update the test scripts to usestart-server-and-testto automate this prerequisite.playgrounds/autocomplete/package.json (1)
6-12: E2E tests require dev server to be running but turbo.json does not establish this dependency.E2E tests expect the dev server at
http://localhost:5173(per cypress.env.json), yet thetest:e2etask in turbo.json only depends on"build"and not on"dev". Runningpnpm test:e2ewill fail because cypress cannot connect to a non-existent dev server. The GitHub workflows work around this by explicitly starting the playground via the cypress-io/github-action, but local or standard CI execution will not have the dev server running.Add
"dev"as a dependency to bothtest:e2eandtest:e2e:watchtasks in turbo.json to ensure the dev server starts before E2E tests run.
🧹 Nitpick comments (1)
playgrounds/local-react/README.md (1)
1-1: Optional: clarify the title to “Local React Playground” (currently generic).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (22)
.github/actions/set-up-node/action.yml(1 hunks).github/dependabot.yml(1 hunks).github/workflows/dependabot-auto-merge.yml(0 hunks).github/workflows/meilisearch-prototype-tests.yml(3 hunks).github/workflows/pre-release-tests.yml(3 hunks).github/workflows/publish.yml(2 hunks).github/workflows/tests.yml(4 hunks)CONTRIBUTING.md(7 hunks)package.json(2 hunks)packages/autocomplete-client/README.md(1 hunks)packages/autocomplete-client/package.json(1 hunks)packages/instant-meilisearch/README.md(1 hunks)playgrounds/autocomplete/README.md(1 hunks)playgrounds/autocomplete/package.json(2 hunks)playgrounds/javascript/README.md(1 hunks)playgrounds/local-react/README.md(1 hunks)playgrounds/local-react/package.json(2 hunks)playgrounds/node-env/package.json(1 hunks)playgrounds/react/README.md(1 hunks)playgrounds/react/package.json(1 hunks)playgrounds/vue3/package.json(1 hunks)pnpm-workspace.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/dependabot-auto-merge.yml
🧰 Additional context used
🪛 LanguageTool
CONTRIBUTING.md
[grammar] ~164-~164: Use a hyphen to join words.
Context: ... for all affected packages. See more in depth explaination on [versioning](https...
(QB_NEW_EN_HYPHEN)
[grammar] ~164-~164: Ensure spelling is correct
Context: ...l affected packages. See more in depth explaination on [versioning](https://github.com/chan...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
playgrounds/local-react/README.md
9-9: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
CONTRIBUTING.md
99-99: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
104-104: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
playgrounds/autocomplete/README.md
9-9: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
packages/autocomplete-client/README.md
57-57: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
playgrounds/react/README.md
9-9: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
playgrounds/javascript/README.md
9-9: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (22)
playgrounds/vue3/package.json (1)
10-16: LGTM:workspace:*aligns with monorepo dependency resolution. Just ensure the workspace package exists / is included by the workspace globs.packages/instant-meilisearch/README.md (1)
116-126: Docs update topnpm addlooks good.playgrounds/react/package.json (1)
19-25: LGTM:workspace:*is the right move for a monorepo playground.playgrounds/local-react/package.json (1)
22-24:workspace:*in a private playground devDependency looks good..github/dependabot.yml (1)
10-41: Dependabot grouping looks good; double-check monthly cadence is intentional.
Switching npm updates to monthly can increase drift/security exposure; if intended, LGTM.playgrounds/autocomplete/package.json (1)
21-27:workspace:*dependency for the local playground is a good monorepo alignment.CONTRIBUTING.md (1)
29-53: Docs command updates to pnpm look consistent.packages/autocomplete-client/package.json (1)
43-45: The workspace protocol handling is already managed by your release tooling. The project uses@changesets/cli, which automatically convertsworkspace:*to actual semver versions during thechangeset publishcommand (as configured in the rootpackage.jsonrelease script). No additional changes are needed; this is the expected and proper configuration for monorepo publishing.package.json (2)
27-28: Scripts and package manager correctly migrated to pnpm.All yarn commands have been properly converted to pnpm equivalents. pnpm 10.25.0 is the latest version, and the packageManager field correctly specifies this for Corepack-based enforcement across all environments.
Also applies to: 30-31, 34-34, 36-36, 64-64
38-63: Verify that E2E tests function correctly after removingconcurrentlydependency.The removal of the
concurrentlypackage aligns with the simplified E2E test execution pattern described in the PR (no concurrent dev server startup). Ensure that the test execution flow in the updated workflows accommodates the sequential or alternative startup approach..github/workflows/publish.yml (2)
30-34: Changesets commands properly updated to pnpm.The
publishandversioncommands are correctly converted from yarn to pnpm, maintaining the same semantics.
19-19: The./.github/actions/set-up-nodecomposite action is properly implemented and correctly handles all Node.js setup, pnpm caching, and registry configuration. No changes needed..github/workflows/pre-release-tests.yml (3)
45-45: Verify that the./.github/actions/set-up-nodecomposite action correctly accepts and uses thenode-versioninput.The action is now passed a
node-versionparameter via thewith:block. Ensure the action properly configures the specified Node.js version before running tests.Also applies to: 80-80, 118-118, 120-120
114-114: Node.js version matrix expanded to 20, 22, 24.Node 18 has been removed from the test matrix. Confirm this aligns with your project's support policy—if Node 18 compatibility should still be maintained, it must remain in the matrix.
Also applies to: 115-115
52-52: pnpm commands correctly applied across test and playground execution.All yarn references have been properly substituted with pnpm equivalents for playground startup and test execution.
Also applies to: 87-87, 122-122
.github/workflows/meilisearch-prototype-tests.yml (3)
47-47: Composite action usage and node-version parameter handling are consistent with other workflows.These changes follow the same pattern as pre-release-tests.yml. Ensure the set-up-node action correctly handles the
node-versioninput parameter.Also applies to: 81-81, 118-118, 120-120
54-54: pnpm commands properly substituted across all test and playground invocations.Yarn to pnpm migration is consistent with the broader PR changes.
Also applies to: 88-88, 122-122
114-114: Node.js version matrix aligned with pre-release-tests.yml (versions 20, 22, 24).Confirm that dropping Node 18 aligns with your project's support policy.
Also applies to: 115-115
.github/workflows/tests.yml (4)
36-36: Composite action consistently used for Node.js and pnpm setup across all jobs.The set-up-node action is applied uniformly to all test jobs. Verify its implementation handles node-version input and pnpm cache correctly.
Also applies to: 80-80, 127-127, 141-141, 153-153
41-41: Clarify the wait-on configuration for cypress-autocomplete-client-run.Line 41 specifies
wait-on: 'http://localhost:7700,http://localhost:5173', whereas the cypress-instant-meilisearch-run job (line 85) only waits forhttp://localhost:7700. The inclusion of port 5173 (Vite dev server) appears inconsistent with the PR's goal of simplifying E2E test execution by removing concurrent dev server startup. Verify whether the Vite dev server on port 5173 is required for the autocomplete playground tests or if this can be removed.Also applies to: 43-43, 87-87
131-131: pnpm commands uniformly applied across all test jobs.All yarn references have been correctly converted to pnpm for test execution, build steps, and playground startup.
Also applies to: 135-135, 143-143, 155-155
123-123: Node.js version matrix expanded to 20, 22, 24; Node 18 removed.Confirm this aligns with your project's Node.js support policy. If Node 18 compatibility is still required, it must be added back to the matrix.
Also applies to: 124-124
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/autocomplete-client/__tests__/test.utils.ts (1)
2-2: Verify themeilisearch.MeiliSearchconstructor is part of the supported public API (and keep it DRY).
Ifmeilisearchis an implementation detail/re-export, this could break with package updates; also you can reuseHOST/API_KEYto avoid config drift between clients.-const meilisearchClient = new meilisearch.MeiliSearch({ - host: 'http://localhost:7700', - apiKey: 'masterKey', -}) +const meilisearchClient = new meilisearch.MeiliSearch({ + host: HOST, + apiKey: API_KEY, +})Also applies to: 15-18
playgrounds/autocomplete/package.json (1)
6-12: E2E scripts no longer start the dev server—verify CI/docs cover the new contract.
If the intent is “run Cypress against an already-running Vite server,” this is OK, but it’s a behavioral change that can surprise local users unless workflows/docs clearly startpnpm dev(or similar) beforehand.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
packages/autocomplete-client/__tests__/test.utils.ts(2 hunks)playgrounds/autocomplete/package.json(2 hunks)playgrounds/autocomplete/setup.mjs(1 hunks)playgrounds/local-react/setup.mjs(1 hunks)playgrounds/node-env/index.js(1 hunks)playgrounds/vue3/src/App.vue(1 hunks)vite.config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- playgrounds/vue3/src/App.vue
🧰 Additional context used
🧬 Code graph analysis (4)
playgrounds/autocomplete/setup.mjs (1)
playgrounds/local-react/setup.mjs (1)
client(5-8)
packages/autocomplete-client/__tests__/test.utils.ts (1)
packages/instant-meilisearch/__tests__/assets/utils.ts (1)
meilisearchClient(330-330)
playgrounds/local-react/setup.mjs (1)
playgrounds/autocomplete/setup.mjs (1)
client(4-7)
playgrounds/node-env/index.js (2)
packages/instant-meilisearch/src/client/instant-meilisearch-client.ts (1)
instantMeiliSearch(46-212)playgrounds/react/src/App.jsx (1)
instantMeiliSearch(18-24)
🔇 Additional comments (4)
vite.config.ts (1)
4-7: The currenttest.projects: ['packages/*']configuration is correct and follows Vitest best practices.Vitest 3.0.2 (your repo's version) accepts glob patterns like
packages/*to discover project directories. This glob is standard and properly matches the two packages in your repository. Both packages havevite.config.tsfiles, which Vitest auto-discovers without requiring separatevitest.config.tsfiles. The suggested change topackages/*/vitest.config.{ts,js,mjs,cjs}would not match any files in your setup and is unnecessary. The migration from the deprecatedtest.workspacetotest.projectsis also correctly applied.Likely an incorrect or invalid review comment.
playgrounds/node-env/index.js (1)
1-4: Constructor switch looks consistent; verify Node runs this file as ESM (top-levelawait).
Themeilisearch.MeiliSearchchange matches the new import shape; just ensure this playground is executed in an environment where top-levelawaitis valid forindex.js(e.g.,type: "module"or equivalent).Also applies to: 11-14
playgrounds/local-react/setup.mjs (1)
1-1: LGTM; please sanity-check the@meilisearch/instant-meilisearchexport shape in this repo’s pinned version.
This is a clean mechanical swap, but it depends onmeilisearchbeing a stable named export andmeilisearch.MeiliSearchbeing available.Also applies to: 5-8
playgrounds/autocomplete/package.json (1)
21-30:workspace:*dependency wiring makes sense for a pnpm monorepo; verify publish boundaries.
Since this package is private it’s likely fine, but ensure nothing intended for publishing relies onworkspace:*versions (and confirm@meilisearch/instant-meilisearchis always available in the workspace graph).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
playgrounds/javascript/package.json (1)
14-16: Consider clarifying workspace dependency context in playground documentation.The
workspace:*protocol correctly resolves within the pnpm workspace (confirmed by config:pnpm-workspace.yamlincludesplaygrounds/*andpackages/*). The dependency will fail clearly outside the workspace with an appropriate error, as designed. However, the CONTRIBUTING.md and playgrounds/javascript/README.md could explicitly mention thatpnpm installmust be run from the repository root (not the playground directory alone) for@meilisearch/instant-meilisearchto resolve. Adding a note like "Install from the workspace root:pnpm install" would prevent contributor confusion around the workspace:* dependency requirement.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
.github/actions/set-up-node/action.yml(1 hunks)eslint.config.js(1 hunks)package.json(1 hunks)packages/instant-meilisearch/__tests__/filter.test.ts(1 hunks)playgrounds/autocomplete/package.json(2 hunks)playgrounds/javascript/package.json(1 hunks)playgrounds/local-react/package.json(2 hunks)playgrounds/react/package.json(1 hunks)playgrounds/vue3/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/instant-meilisearch/tests/filter.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- playgrounds/vue3/package.json
- playgrounds/autocomplete/package.json
- playgrounds/react/package.json
- .github/actions/set-up-node/action.yml
🔇 Additional comments (5)
eslint.config.js (1)
90-100: Address the TODO to remove disabled vitest rules; no violations currently exist in the test suite.The
vitest/no-conditional-expectdisable aligns with the existingTODO: Remove rulescomment. Since no violations of this rule (or the other two disabled rules) were found across 27 test files, removing all three disables is safe and recommended:// Vitest { files: ['packages/*/{src,__tests__}/**/*.test.ts'], extends: [vitest.configs.recommended], - // TODO: Remove rules - rules: { - 'vitest/no-identical-title': 'off', - 'vitest/valid-title': 'off', - 'vitest/no-conditional-expect': 'off', - }, },If removing all three causes test failures, enable the failing rule(s) selectively with inline
// eslint-disable-next-lineand a brief reason.playgrounds/local-react/package.json (2)
6-12: Cypress scripts now assume workspace/root toolchain—verifyturbo/pnpm filtering still works.
If CI/dev runs these viaturbo test:e2efrom the repo root, this is fine; but running the package in isolation may now fail unless the repo-level install is enforced. Consider documenting “run from repo root” (or adding a guard in scripts).
22-24: Dropping React/Vite deps here is OK only if intentionally centralized—please confirm no standalone install path.
Right now this playground depends on root workspace deps to build/run; if that’s the intent, LGTM—just ensure docs/CI reflect it and no tooling expects per-package installs.package.json (2)
27-36: Scripts look consistent; verify they run correctly under pnpm in CI (esp. build chaining).
style/style:fix/test/test:watch/test:types/releasenow assume pnpm is the invoker; please ensure CI usespnpm -w(orcorepack pnpm) and thatpnpm build(root script) behaves as expected across platforms.
73-73: CI already honorspnpm@10.25.0viapnpm/action-setup.The workflows use
pnpm/action-setup@v4, which automatically respects thepackageManagerfield inpackage.jsonwithout requiring explicit corepack or version configuration. The pinned version is enforced in CI.For local development, consider documenting in
CONTRIBUTING.md(or similar) that contributors should either enable corepack or explicitly installpnpm@10.25.0to match the pinned version, avoiding accidental use of a different major version.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
package.json(1 hunks)playgrounds/autocomplete/package.json(2 hunks)
🔇 Additional comments (3)
package.json (2)
26-27: LGTM! Script migrations are consistent.The yarn-to-pnpm migrations across
style,test, andreleasescripts are correctly implemented and maintain the same logical flow.Also applies to: 29-30, 33-33, 35-35
73-73: No action needed—pnpm@10.26.0 is a valid and stable version.The specified version (10.26.0) is the latest stable release, published 2 days ago. It was released on December 15 and is appropriate for use with Corepack.
playgrounds/autocomplete/package.json (1)
10-11: LGTM! Test scripts correctly migrated to pnpm.The e2e test scripts now properly use
pnpm devinstead ofyarn dev, consistent with the package manager migration.
flevi29
changed the title
yarn to pnpm package manager
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/instant-meilisearch/__tests__/search-resolver.test.ts (1)
22-35: Critical: Tests are failing due to broken mock setup.The pipeline shows 5 test failures with
TypeError: () => value is not a constructor. The issue is thatMeiliSearchneeds to be mocked as a constructor, but the current setup doesn't achieve this correctly.The pattern
vi.mocked(MeiliSearch, true).mockReturnValue(...)doesn't work because:
vi.mocked()is a type assertion helper, not a mock creatorMeiliSearchis a class constructor that needs to be mocked with proper constructor semantics🔎 Suggested fix for the mock setup:
vi.mock('meilisearch') -// Mocking of Meilisearch package -const mockedMeilisearch = vi.mocked(MeiliSearch, true) const mockedMultiSearch = vi.fn(function (request) { const response = request.queries.map((req: MeiliSearchMultiSearchParams) => ({ ...searchResponse, indexUid: req.indexUid, })) return { results: response, } }) -// @ts-ignore -mockedMeilisearch.mockReturnValue({ - multiSearch: mockedMultiSearch, -}) +vi.mocked(MeiliSearch).mockImplementation(() => { + return { + multiSearch: mockedMultiSearch, + } as any +})Alternative approach using
mockReturnValueon the constructor itself:vi.mock('meilisearch', () => { return { - MeiliSearch: vi.fn() + MeiliSearch: vi.fn().mockImplementation(() => ({ + multiSearch: mockedMultiSearch, + })) } }) - -const mockedMeilisearch = vi.mocked(MeiliSearch, true) -// ... -// @ts-ignore -mockedMeilisearch.mockReturnValue({ - multiSearch: mockedMultiSearch, -})
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/instant-meilisearch/__tests__/custom-http-client.test.ts(1 hunks)packages/instant-meilisearch/__tests__/filter.test.ts(1 hunks)packages/instant-meilisearch/__tests__/search-resolver.test.ts(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Tests
packages/instant-meilisearch/__tests__/search-resolver.test.ts
[error] 49-49: Test failed: TypeError: () => value is not a constructor
[error] 75-75: Test failed: TypeError: () => value is not a constructor
[error] 101-101: Test failed: TypeError: () => value is not a constructor
[error] 128-128: Test failed: TypeError: () => value is not a constructor
[error] 164-164: Test failed: TypeError: () => value is not a constructor
🔇 Additional comments (1)
packages/instant-meilisearch/__tests__/custom-http-client.test.ts (1)
13-19: LGTM!The change from arrow function to function expression for the mock is semantically equivalent in this context. The mock behavior remains unchanged.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/instant-meilisearch/__tests__/search-resolver.test.ts (1)
19-33: Critical: Fix the module mock setup to resolve TDZ error and incorrect mocking strategy.The pipeline failure confirms this code has multiple critical issues:
- Temporal Dead Zone (TDZ) error:
vi.mock()is hoisted and executes before line 20, somockedMultiSearch(line 32) is accessed before initialization.- Incorrect syntax: Line 30 uses
vi.mock(import('meilisearch'), ...)but the first argument must be a string:vi.mock('meilisearch', ...).- Wrong mocking strategy: The mock adds
multiSearchas a module export, butmultiSearchis an instance method of theMeiliSearchclass, not a module-level export.- Inconsistent setup: Line 19 uses
vi.mocked()on the class, which conflicts with the module-level mock approach.🔎 Proposed fix for the mock setup
-// Mocking of Meilisearch package -const mockedMeilisearch = vi.mocked(MeiliSearch, true) -const mockedMultiSearch = vi.fn(function (request) { - const response = request.queries.map((req: MeiliSearchMultiSearchParams) => ({ - ...searchResponse, - indexUid: req.indexUid, - })) - return { - results: response, - } -}) - -vi.mock(import('meilisearch'), async (originalImport) => ({ - ...(await originalImport()), - multiSearch: mockedMultiSearch, -})) +// Mocking of Meilisearch package +const mockedMultiSearch = vi.fn(function (request) { + const response = request.queries.map((req: MeiliSearchMultiSearchParams) => ({ + ...searchResponse, + indexUid: req.indexUid, + })) + return { + results: response, + } +}) + +vi.mock('meilisearch', () => { + return { + MeiliSearch: vi.fn().mockImplementation(() => ({ + multiSearch: mockedMultiSearch, + })), + } +}) + +const mockedMeilisearch = vi.mocked(MeiliSearch)This fix:
- Moves the mock definition before variable references (though
vi.mock()is still hoisted, the factory function will execute after)- Uses correct syntax:
vi.mock('meilisearch', ...)with a string- Mocks the
MeiliSearchclass constructor to return an object with themultiSearchmethod- Keeps
mockedMeilisearchfor the existing assertions
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/instant-meilisearch/__tests__/search-resolver.test.ts(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Tests
packages/instant-meilisearch/__tests__/search-resolver.test.ts
[error] 32-32: Vitest mocking error: Cannot access 'mockedMultiSearch' before initialization. See test setup in tests/search-resolver.test.ts.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/instant-meilisearch/__tests__/search-resolver.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/instant-meilisearch/__tests__/search-resolver.test.ts (1)
packages/instant-meilisearch/src/cache/__tests__/assets/utils.ts (1)
searchResponse(1-9)
🪛 GitHub Actions: Tests
packages/instant-meilisearch/__tests__/search-resolver.test.ts
[error] 1-1: Vitest mocking error: There was an error when mocking a module. If you are using 'vi.mock' factory, make sure there are no top level variables inside, since this call is hoisted to top of the file. Caused by: ReferenceError: Cannot access 'mockedMeilisearch' before initialization
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/instant-meilisearch/__tests__/search-resolver.test.ts (1)
20-35: Consider improving type safety on the mock return value.The
as anytype assertion on line 33 bypasses type checking. While this works for testing purposes, consider typing the return value properly or adding a comment explaining why the assertion is necessary.Possible improvement
If the return type doesn't match exactly, you could:
- Add a comment explaining the type mismatch
- Type it as
Partial<ReturnType<MeiliSearch['multiSearch']>>if only partial properties are needed- Create a test-specific type that matches your mock structure
- return { + return Promise.resolve({ results: response, - } as any + })Note: Verify if
multiSearchreturns a Promise in the actual implementation.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/instant-meilisearch/__tests__/search-resolver.test.ts(6 hunks)
🔇 Additional comments (3)
packages/instant-meilisearch/__tests__/search-resolver.test.ts (3)
18-19: Mock setup correctly addresses the previous hoisting issue.The dynamic import mock with
{ spy: true }properly resolves the ReferenceError that was flagged in the previous review. This approach allows per-instance spying without hoisting order problems.
49-52: Consistent test setup pattern improves maintainability.All tests follow the same pattern of obtaining both
searchClientandmeiliSearchInstance, then creating a fresh spy. This consistency makes the test suite easy to understand and maintain.Also applies to: 79-82, 109-112, 140-143, 180-183
57-61: Constructor assertions properly verify configuration.The assertions confirm that the
MeiliSearchconstructor is called with the correct configuration including host, apiKey, and clientAgents. This ensures proper instantiation across all test scenarios.Also applies to: 87-91, 118-122, 150-154, 190-194
Strift
left a comment
Strift
left a comment
There was a problem hiding this comment.
awesome stuff, let's get this merged 🙌
|
I'll rebase. Don't know how this keeps happening. |
flevi29
force-pushed
the
yarn-to-pnpm
branch
from
97a6ba3 to
d897a85
Compare
Pull Request
What does this PR do?
yarntopnpm, adapted code and configpackage.json, to avoid duplicates with different versions"@meilisearch/geo-playground"Summary by CodeRabbit
Chores
Removed
✏️ Tip: You can customize this high-level summary in your review settings.