BA-2130: organize mocks #187
Conversation
|
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/design-system/__mocks__/events.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/design-system/.eslintrc.js
WalkthroughThis pull request involves a comprehensive update of mock files across multiple packages in the Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
packages/test/__mocks__/expo-constants.ts (3)
1-9: Add TypeScript type definitions for better type safety.Consider adding type definitions to match the actual
expo-constantsmodule structure. This will help catch type-related issues during testing.+import { ExpoConfig } from 'expo-constants'; +interface MockConstants { + expoConfig: { + extra: { + EXPO_PUBLIC_API_BASE_URL: undefined; + EXPO_PUBLIC_RELAY_ENDPOINT: undefined; + EXPO_PUBLIC_WS_RELAY_ENDPOINT: undefined; + }; + }; +} -const Constants = { +const Constants: MockConstants = { expoConfig: { extra: { EXPO_PUBLIC_API_BASE_URL: undefined, EXPO_PUBLIC_RELAY_ENDPOINT: undefined, EXPO_PUBLIC_WS_RELAY_ENDPOINT: undefined, }, }, }
1-9: Consider making the mock more configurable for different test scenarios.Instead of hardcoding
undefinedvalues, consider making the mock more flexible by allowing values to be set during tests.-const Constants = { +const createMockConstants = (config = {}) => ({ expoConfig: { extra: { - EXPO_PUBLIC_API_BASE_URL: undefined, - EXPO_PUBLIC_RELAY_ENDPOINT: undefined, - EXPO_PUBLIC_WS_RELAY_ENDPOINT: undefined, + EXPO_PUBLIC_API_BASE_URL: 'http://localhost:3000', + EXPO_PUBLIC_RELAY_ENDPOINT: 'http://localhost:3000/graphql', + EXPO_PUBLIC_WS_RELAY_ENDPOINT: 'ws://localhost:3000/graphql', + ...config, }, }, -} +}); + +const Constants = createMockConstants();
11-11: Consider using modern ES module syntax.The codebase might benefit from consistent use of ES module syntax across files.
-module.exports = Constants +export default Constants;packages/test/README.md (1)
25-25: LGTM! Consider adding a brief description of each mock's purpose.The simplified naming convention is more consistent. However, it would be helpful to briefly describe what each mock does (e.g., "Mocks console methods to prevent test output noise").
Also applies to: 28-28
packages/test/__mocks__/expo-secure-store.ts (1)
13-13: Use ES modules export syntax.For consistency with TypeScript, use ES modules export syntax instead of CommonJS.
-module.exports = ExpoSecureStore +export default ExpoSecureStorepackages/design-system/__mocks__/events.ts (1)
17-21: Improve type safety and add return value for off method.The
offmethod needs proper typing and should indicate whether the listener was successfully removed.- off(event: string, listener: Function) { + off(event: string, listener: EventListener): boolean { + const listeners = this.events[event] + if (!listeners?.length) { + return false + } - if (this.events[event]) { - this.events[event] = this.events[event].filter((l) => l !== listener) - } + const initialLength = listeners.length + this.events[event] = listeners.filter((l) => l !== listener) + return this.events[event].length < initialLength }🧰 Tools
🪛 Biome (1.9.4)
[error] 17-17: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
packages/design-system/.storybook/main.ts (1)
65-71: LGTM! Consistent mock organization across packages.The mock file organization follows the same pattern as other packages, improving maintainability. The use of
resolve()ensures proper path resolution.Consider consolidating NextImage mock location.
While most mocks are in the parent
__mocks__directory,NextImage.tsxremains in.storybook/__mocks__. Consider moving it to maintain consistency with other mocks.packages/wagtail/__mocks__/react-native.ts (2)
1-1: Consider using ES module syntax for consistency.While the current CommonJS
requireimplementation works, consider using ES module syntax for better consistency with modern TypeScript practices:-module.exports = require('@baseapp-frontend/test/__mocks__/react-native.ts') +export * from '@baseapp-frontend/test/__mocks__/react-native.ts'
1-3: Consider documenting the mock organization pattern.This file follows a good pattern of centralizing common mocks in
@baseapp-frontend/testand re-exporting them in individual packages. To help maintain this structure:
- Document this pattern in the project's contribution guidelines
- Consider creating a template for new mock files
- Add tests to verify that package-specific mocks don't duplicate functionality from the central mocks
Would you like me to help create this documentation or generate a template for new mock files?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between ddf0748 and d94e9b8fd80afd9fe46a000634c93574dd9b00ee.
📒 Files selected for processing (59)
packages/authentication/__mocks__/console.ts(1 hunks)packages/authentication/__mocks__/expo-constants.ts(1 hunks)packages/authentication/__mocks__/expo-modules-core.ts(1 hunks)packages/authentication/__mocks__/expo-secure-store.ts(1 hunks)packages/authentication/__mocks__/expo.ts(0 hunks)packages/authentication/__mocks__/fetch.ts(1 hunks)packages/authentication/__mocks__/file.ts(1 hunks)packages/authentication/__mocks__/fileMock.ts(0 hunks)packages/authentication/__mocks__/style.ts(1 hunks)packages/components/.storybook/__mocks__/nextFontMock.ts(0 hunks)packages/components/.storybook/main.ts(1 hunks)packages/components/__mocks__/console.ts(1 hunks)packages/components/__mocks__/expo-constants.ts(1 hunks)packages/components/__mocks__/expo-modules-core.ts(1 hunks)packages/components/__mocks__/expo-secure-store.ts(1 hunks)packages/components/__mocks__/expo.ts(0 hunks)packages/components/__mocks__/fetch.ts(1 hunks)packages/components/__mocks__/file.ts(1 hunks)packages/components/__mocks__/fileMock.ts(0 hunks)packages/components/__mocks__/next-font.ts(1 hunks)packages/components/__mocks__/nextFontMock.ts(0 hunks)packages/components/__mocks__/style.ts(1 hunks)packages/components/cypress/__mocks__/nextFontMock.ts(0 hunks)packages/components/jest.config.ts(1 hunks)packages/components/webpack.config.ts(2 hunks)packages/config/relay.config.ts(1 hunks)packages/design-system/.storybook/__mocks__/next-font.ts(1 hunks)packages/design-system/.storybook/__mocks__/nextFontMock.ts(0 hunks)packages/design-system/.storybook/main.ts(1 hunks)packages/design-system/__mocks__/events.ts(1 hunks)packages/design-system/__mocks__/expo-constants.ts(1 hunks)packages/design-system/__mocks__/expo-modules-core.ts(1 hunks)packages/design-system/__mocks__/expo-secure-store.ts(1 hunks)packages/design-system/__mocks__/next-font.ts(1 hunks)packages/design-system/__mocks__/react-native.ts(1 hunks)packages/test/README.md(1 hunks)packages/test/__mocks__/expo-constants.ts(1 hunks)packages/test/__mocks__/expo-modules-core.ts(1 hunks)packages/test/__mocks__/expo-secure-store.ts(1 hunks)packages/test/__mocks__/expo.ts(0 hunks)packages/test/index.tsx(1 hunks)packages/test/jest.config.ts(1 hunks)packages/utils/__mocks__/console.ts(1 hunks)packages/utils/__mocks__/expo-constants.ts(1 hunks)packages/utils/__mocks__/expo-modules-core.ts(1 hunks)packages/utils/__mocks__/expo-secure-store.ts(1 hunks)packages/utils/__mocks__/expo.ts(0 hunks)packages/utils/__mocks__/fetch.ts(1 hunks)packages/utils/__mocks__/file.ts(1 hunks)packages/utils/__mocks__/fileMock.ts(0 hunks)packages/utils/__mocks__/style.ts(1 hunks)packages/wagtail/.storybook/__mocks__/nextFontMock.ts(0 hunks)packages/wagtail/.storybook/main.ts(1 hunks)packages/wagtail/__mocks__/expo-constants.ts(1 hunks)packages/wagtail/__mocks__/expo-modules-core.ts(1 hunks)packages/wagtail/__mocks__/expo-secure-store.ts(1 hunks)packages/wagtail/__mocks__/next-font.ts(1 hunks)packages/wagtail/__mocks__/react-native.ts(1 hunks)packages/wagtail/webpack.config.ts(2 hunks)
💤 Files with no reviewable changes (12)
- packages/utils/mocks/expo.ts
- packages/authentication/mocks/fileMock.ts
- packages/components/mocks/expo.ts
- packages/components/mocks/fileMock.ts
- packages/components/.storybook/mocks/nextFontMock.ts
- packages/wagtail/.storybook/mocks/nextFontMock.ts
- packages/utils/mocks/fileMock.ts
- packages/authentication/mocks/expo.ts
- packages/components/mocks/nextFontMock.ts
- packages/components/cypress/mocks/nextFontMock.ts
- packages/design-system/.storybook/mocks/nextFontMock.ts
- packages/test/mocks/expo.ts
✅ Files skipped from review due to trivial changes (34)
- packages/wagtail/mocks/next-font.ts
- packages/components/mocks/file.ts
- packages/design-system/mocks/react-native.ts
- packages/utils/mocks/expo-secure-store.ts
- packages/design-system/mocks/expo-secure-store.ts
- packages/design-system/mocks/expo-modules-core.ts
- packages/test/mocks/expo-modules-core.ts
- packages/authentication/mocks/console.ts
- packages/components/mocks/style.ts
- packages/authentication/mocks/file.ts
- packages/authentication/mocks/expo-modules-core.ts
- packages/authentication/mocks/fetch.ts
- packages/components/mocks/expo-secure-store.ts
- packages/utils/mocks/fetch.ts
- packages/components/mocks/console.ts
- packages/utils/mocks/expo-constants.ts
- packages/design-system/mocks/next-font.ts
- packages/test/index.tsx
- packages/wagtail/mocks/expo-constants.ts
- packages/design-system/mocks/expo-constants.ts
- packages/components/mocks/expo-constants.ts
- packages/components/mocks/fetch.ts
- packages/utils/mocks/file.ts
- packages/utils/mocks/console.ts
- packages/wagtail/mocks/expo-modules-core.ts
- packages/authentication/mocks/expo-secure-store.ts
- packages/authentication/mocks/style.ts
- packages/design-system/.storybook/mocks/next-font.ts
- packages/wagtail/mocks/expo-secure-store.ts
- packages/components/mocks/next-font.ts
- packages/utils/mocks/expo-modules-core.ts
- packages/utils/mocks/style.ts
- packages/authentication/mocks/expo-constants.ts
- packages/components/mocks/expo-modules-core.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/design-system/__mocks__/events.ts
[error] 2-2: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 4-4: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 17-17: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
🪛 GitHub Actions: Main Workflow
packages/design-system/__mocks__/events.ts
[error] 8-8: Object is possibly 'undefined'
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (13)
packages/components/jest.config.ts (2)
7-7: LGTM! The mock file naming follows the new convention.The rename from
graphqlWsMock.tstographql-ws.tsaligns with the standardized naming pattern where mock files match their module names.
7-7: Verify the mock file exists at the new location.Let's ensure the mock file has been properly moved/renamed.
✅ Verification successful
Mock file verified at the correct location
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the mock file and check for any remaining references to the old name. # Check if the new mock file exists fd "graphql-ws.ts" "packages/components/jest/__mocks__" # Search for any remaining references to the old name that might need updating rg -g "!*.md" "graphqlWsMock"Length of output: 132
packages/test/README.md (2)
33-33: LGTM! Naming change is consistent with the new convention.Also applies to: 36-36
41-41: Verify mock file references across dependent packages.The naming change is consistent with the new convention. Let's verify that all references to these mocks have been updated across the monorepo.
Also applies to: 44-44
✅ Verification successful
Mock file references are consistent across packages ✅
All mock files follow the new naming convention in the following locations:
- packages/utils/mocks/
- packages/authentication/mocks/
- packages/components/mocks/
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for any remaining references to old mock filenames # that might have been missed during the rename echo "Checking for old mock references..." rg -l "(console|file|style)Mock\.ts" echo "Verifying new mock files exist in other packages..." fd -g "*/__mocks__/(console|file|style).ts"Length of output: 278
Script:
#!/bin/bash # Search for any import/require statements referencing these mocks echo "Checking for mock imports/requires..." rg "require\(['\"].*/__mocks__/(console|file|style)(Mock)?\.ts['\"]" -l rg "from ['\"].*/__mocks__/(console|file|style)(Mock)?\.ts['\"]" -l # Find all mock files with these names echo "Locating mock files..." fd "(console|file|style)(Mock)?\.ts" -p "__mocks__" # Check Jest configurations echo "Checking Jest configs..." fd "jest.config" -e js -e json -e tsLength of output: 1075
packages/config/relay.config.ts (1)
4-10: LGTM! Excluding cypress directory from Relay processing.The addition of the
cypressexclusion pattern is appropriate as Relay doesn't need to process E2E test files.Let's verify that we haven't missed any other test-related directories that should be excluded:
✅ Verification successful
Relay exclusion patterns are complete and appropriate
The verification shows that the current exclusion patterns, including the new
cypressdirectory, are sufficient. Only one test directory contains GraphQL usage, and it's already covered by existing exclusions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find potential test-related directories that might need to be excluded # Search for test-related directories that might contain GraphQL operations fd -t d -E node_modules -E .next -E __generated__ -E .app-templates -E cypress \ -g '*test*' -g '*spec*' -g '*mock*' -g '*fixture*' \ | while read -r dir; do if rg -l 'graphql`|gql`' "$dir" 2>/dev/null; then echo "Found GraphQL usage in test directory: $dir" fi doneLength of output: 348
Script:
#!/bin/bash # Search for test-related directories and check for GraphQL usage fd -t d -E node_modules -E .next -E __generated__ -E .app-templates -E cypress \ -g '*test*' -o -g '*spec*' -o -g '*mock*' -o -g '*fixture*' . \ | while read -r dir; do if rg --type-add 'test:*.{js,jsx,ts,tsx}' --type test -l 'graphql`|gql`' "$dir" 2>/dev/null; then echo "Directory with GraphQL usage: $dir" fi done # Also check if there are any other common test directories we should consider fd -t d -d 1 . \ | grep -E '(test|spec|mock|fixture|e2e|integration)'Length of output: 343
Script:
#!/bin/bash # Find test-related directories and check for GraphQL usage find . -type d \( \ -name "test" -o \ -name "tests" -o \ -name "__tests__" -o \ -name "e2e" -o \ -name "integration" \ \) -not -path "*/node_modules/*" \ -not -path "*/.next/*" \ -not -path "*/__generated__/*" \ -not -path "*/.app-templates/*" \ -not -path "*/cypress/*" \ | while read -r dir; do if [ -d "$dir" ] && rg -l 'graphql`|gql`' "$dir" 2>/dev/null; then echo "Found GraphQL usage in: $dir" fi doneLength of output: 7341
packages/test/jest.config.ts (1)
16-24: LGTM! Mock file organization looks good.The mock file paths have been consistently updated and new mappings for expo packages have been added. This aligns well with the PR objective of organizing mocks.
packages/wagtail/webpack.config.ts (1)
17-22: LGTM! Mock resolution configuration is well-structured.The alias mappings are properly configured using
require.resolvefor absolute paths, and the mock file paths are consistent with the Jest configuration.Also applies to: 54-54
packages/components/webpack.config.ts (2)
18-18: LGTM! Mock paths are well-organized.The reorganization of mock files into a dedicated
__mocks__directory follows best practices for test file organization. The consistent naming convention (events.ts,expo-*.ts, etc.) improves maintainability.Also applies to: 21-24
58-58: Verify mock file existence and consistency.Ensure that the new path for the next-font mock exists and maintains the same functionality as the previous implementation.
✅ Verification successful
Mock implementation verified and consistent ✅
The next-font mock exists and follows the established pattern of re-exporting from the centralized mock implementation, ensuring consistency across packages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the mock file exists and verify its exports if [ -f "packages/components/__mocks__/next-font.ts" ]; then echo "Mock file exists" cat "packages/components/__mocks__/next-font.ts" else echo "ERROR: Mock file not found" exit 1 fiLength of output: 232
Script:
#!/bin/bash # Search for the source mock file fd "next-font.ts" --type f --exec grep -l "@baseapp-frontend/test/__mocks__" {} \; fd "next-font.ts" --type f --exec cat {} \;Length of output: 665
packages/wagtail/.storybook/main.ts (1)
69-75: LGTM! Mock configuration aligns with other packages.The mock file organization and path resolution are consistent with other packages in the monorepo, which is excellent for maintainability.
packages/components/.storybook/main.ts (1)
71-77: LGTM! Mock configuration maintains consistency.The mock file organization and path resolution maintain consistency with other packages, which is excellent for maintainability.
Verify mock file synchronization across packages.
Since multiple packages are using similar mocks, ensure they remain synchronized.
✅ Verification successful
Mock synchronization is properly maintained with a centralized approach
The verification shows that most mocks are properly synchronized through imports from the test package, which serves as the single source of truth. One minor optimization opportunity: consider centralizing the
events.tsmock in the test package like other mocks, as it currently has identical implementations across packages.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if mock files are identical across packages for mock in expo-constants.ts expo-modules-core.ts expo-secure-store.ts events.ts react-native.ts next-font.ts; do echo "Checking $mock across packages..." # Find all instances of the mock file fd -t f "$mock" packages/ # Compare content if multiple instances exist if [ $(fd -t f "$mock" packages/ | wc -l) -gt 1 ]; then echo "Multiple instances found. Comparing content..." for file in $(fd -t f "$mock" packages/); do echo "=== $file ===" cat "$file" echo "===============" done fi doneLength of output: 16908
packages/wagtail/__mocks__/react-native.ts (2)
3-3: Verify if the empty export is still needed after switching to ES modules.The empty export statement is currently used to ensure TypeScript treats this as a module. If you switch to the ES module syntax as suggested above, this empty export would become redundant.
✅ Verification successful
Keep the empty export statement as it's essential for mock files
The empty export is still needed as it ensures proper module scoping for mocks, which is a consistent pattern used across all 31 mock files in the project. This pattern serves a specific purpose for mock isolation and should be maintained even with ES modules.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if other mock files in the project use similar empty exports rg -l "export \{\}" --type ts packages/**/__mocks__/ # Check the TypeScript configuration that might require this pattern fd "tsconfig.*json" -x cat {} \; | jq -r 'select(.compilerOptions.esModuleInterop != null) | "Found esModuleInterop setting in: " + input_filename'Length of output: 1931
1-3: Verify the mock implementation is complete.Since this file re-exports mocks from a centralized location, we should verify that all necessary React Native components and APIs used in the wagtail package are properly mocked.
✅ Verification successful
Mock implementation is appropriately minimal
The wagtail package doesn't use any React Native components or APIs in its source code. The current minimal mock implementation that only provides
Platform.OSis sufficient, as explicitly intended by the comment "ignoring react-native imports in tests" in the source mock file.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find React Native imports in wagtail package echo "React Native imports in wagtail package:" rg -A 1 "from 'react-native'" packages/wagtail/src # Check the source mock file to ensure coverage echo -e "\nMocked exports in source file:" rg "export const|export function" "@baseapp-frontend/test/__mocks__/react-native.ts"Length of output: 461
Script:
#!/bin/bash # First, find the correct paths echo "Finding wagtail source directory:" fd -t d "wagtail/src$" echo -e "\nLocating the source mock file:" fd -t f "react-native.ts$" -E "node_modules" echo -e "\nIf paths are found, examine React Native imports:" fd -t d "wagtail/src$" -x rg -l "from 'react-native'"Length of output: 669
Script:
#!/bin/bash # Search for React Native imports in the entire wagtail package echo "React Native imports in wagtail package:" rg "from 'react-native'" packages/wagtail -l echo -e "\nContent of wagtail mock file:" cat packages/wagtail/__mocks__/react-native.ts echo -e "\nContent of test mock file:" cat packages/test/__mocks__/react-native.tsLength of output: 594
Script:
#!/bin/bash # Search for React Native imports in tsx files and alternative patterns echo "Searching for React Native imports in wagtail package (including .tsx):" rg "from ['\"]react-native['\"]|import.*['\"]react-native['\"]" packages/wagtail -l echo -e "\nSearching for specific React Native components/APIs:" rg "Platform\.|StyleSheet\.|View,|Text,|TouchableOpacity" packages/wagtail -lLength of output: 492
| const ExpoSecureStore = { | ||
| getItemAsync: async (key: string) => { | ||
| return key === 'ACCESS_KEY_NAME' ? 'mocked_value' : null | ||
| }, | ||
| setItemAsync: async (key: string, value: string) => { | ||
| return true | ||
| }, | ||
| deleteItemAsync: async (key: string) => { | ||
| return true | ||
| }, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance mock implementation with error scenarios and state management.
The current mock implementation is overly simplistic and may not adequately test error handling in consuming code.
Consider enhancing the mock with:
- In-memory storage to track key-value pairs
- Error simulation for common scenarios
- Type definitions for the methods
-const ExpoSecureStore = {
+interface SecureStore {
+ getItemAsync(key: string): Promise<string | null>
+ setItemAsync(key: string, value: string): Promise<boolean>
+ deleteItemAsync(key: string): Promise<boolean>
+}
+
+const store = new Map<string, string>()
+
+const ExpoSecureStore: SecureStore = {
getItemAsync: async (key: string) => {
- return key === 'ACCESS_KEY_NAME' ? 'mocked_value' : null
+ return store.get(key) ?? null
},
setItemAsync: async (key: string, value: string) => {
+ if (!key || !value) {
+ throw new Error('Invalid key or value')
+ }
+ store.set(key, value)
return true
},
deleteItemAsync: async (key: string) => {
+ if (!store.has(key)) {
+ return false
+ }
+ store.delete(key)
return true
},
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const ExpoSecureStore = { | |
| getItemAsync: async (key: string) => { | |
| return key === 'ACCESS_KEY_NAME' ? 'mocked_value' : null | |
| }, | |
| setItemAsync: async (key: string, value: string) => { | |
| return true | |
| }, | |
| deleteItemAsync: async (key: string) => { | |
| return true | |
| }, | |
| } | |
| interface SecureStore { | |
| getItemAsync(key: string): Promise<string | null> | |
| setItemAsync(key: string, value: string): Promise<boolean> | |
| deleteItemAsync(key: string): Promise<boolean> | |
| } | |
| const store = new Map<string, string>() | |
| const ExpoSecureStore: SecureStore = { | |
| getItemAsync: async (key: string) => { | |
| return store.get(key) ?? null | |
| }, | |
| setItemAsync: async (key: string, value: string) => { | |
| if (!key || !value) { | |
| throw new Error('Invalid key or value') | |
| } | |
| store.set(key, value) | |
| return true | |
| }, | |
| deleteItemAsync: async (key: string) => { | |
| if (!store.has(key)) { | |
| return false | |
| } | |
| store.delete(key) | |
| return true | |
| }, | |
| } |
| emit(event: string, ...args: any[]) { | ||
| if (this.events[event]) { | ||
| this.events[event].forEach((listener) => listener(...args)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix potential undefined access in emit method.
The pipeline failure indicates a possible undefined access. Additionally, the method could benefit from error handling.
emit(event: string, ...args: any[]) {
+ const listeners = this.events[event]
+ if (!listeners?.length) {
+ return false
+ }
- if (this.events[event]) {
- this.events[event].forEach((listener) => listener(...args))
- }
+ listeners.forEach((listener) => {
+ try {
+ listener(...args)
+ } catch (error) {
+ console.error(`Error in event listener for ${event}:`, error)
+ }
+ })
+ return true
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| emit(event: string, ...args: any[]) { | |
| if (this.events[event]) { | |
| this.events[event].forEach((listener) => listener(...args)) | |
| } | |
| } | |
| emit(event: string, ...args: any[]) { | |
| const listeners = this.events[event] | |
| if (!listeners?.length) { | |
| return false | |
| } | |
| listeners.forEach((listener) => { | |
| try { | |
| listener(...args) | |
| } catch (error) { | |
| console.error(`Error in event listener for ${event}:`, error) | |
| } | |
| }) | |
| return true | |
| } |
| on(event: string, listener: Function) { | ||
| if (!this.events[event]) { | ||
| this.events[event] = [] | ||
| } | ||
| this.events[event].push(listener) | ||
| } |
There was a problem hiding this comment.
Add type safety to event listener registration.
The on method needs proper typing for the listener parameter.
- on(event: string, listener: Function) {
+ on(event: string, listener: EventListener) {
if (!this.events[event]) {
this.events[event] = []
}
this.events[event].push(listener)
+ return this // Enable method chaining
}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 4-4: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
🪛 GitHub Actions: Main Workflow
[error] 8-8: Object is possibly 'undefined'
| export class EventEmitter { | ||
| private events: Record<string, Function[]> = {} | ||
|
|
There was a problem hiding this comment.
Improve type safety by replacing generic Function type.
The use of Function type is discouraged as it's too permissive and can lead to type-related bugs.
+type EventListener = (...args: any[]) => void
+
export class EventEmitter {
- private events: Record<string, Function[]> = {}
+ private events: Record<string, EventListener[]> = {}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export class EventEmitter { | |
| private events: Record<string, Function[]> = {} | |
| type EventListener = (...args: any[]) => void | |
| export class EventEmitter { | |
| private events: Record<string, EventListener[]> = {} |
🧰 Tools
🪛 Biome (1.9.4)
[error] 2-2: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
anicioalexandre
force-pushed
the
BA-2130-organize-mocks
branch
from
d94e9b8 to
ec0ceb1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/wagtail/__mocks__/events.ts (2)
1-2: 🛠️ Refactor suggestionImprove type safety by replacing generic Function type.
The use of
Functiontype is too permissive and can lead to type-related bugs.+type EventListener = (...args: any[]) => void + export class EventEmitter { - private events: Record<string, Function[]> = {} + private events: Record<string, EventListener[]> = {}🧰 Tools
🪛 Biome (1.9.4)
[error] 2-2: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
11-15: 🛠️ Refactor suggestionFix potential undefined access in emit method.
The method needs better error handling and protection against undefined access.
emit(event: string, ...args: any[]) { + const listeners = this.events[event] + if (!listeners?.length) { + return false + } - if (this.events[event]) { - this.events[event]?.forEach((listener) => listener(...args)) - } + listeners.forEach((listener) => { + try { + listener(...args) + } catch (error) { + console.error(`Error in event listener for ${event}:`, error) + } + }) + return true }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between d94e9b8fd80afd9fe46a000634c93574dd9b00ee and ec0ceb122f777686ef3319049cda872f30a08c9b.
📒 Files selected for processing (61)
.changeset/soft-books-sin.md(1 hunks)packages/authentication/__mocks__/console.ts(1 hunks)packages/authentication/__mocks__/expo-constants.ts(1 hunks)packages/authentication/__mocks__/expo-modules-core.ts(1 hunks)packages/authentication/__mocks__/expo-secure-store.ts(1 hunks)packages/authentication/__mocks__/expo.ts(0 hunks)packages/authentication/__mocks__/fetch.ts(1 hunks)packages/authentication/__mocks__/file.ts(1 hunks)packages/authentication/__mocks__/fileMock.ts(0 hunks)packages/authentication/__mocks__/style.ts(1 hunks)packages/components/.storybook/__mocks__/nextFontMock.ts(0 hunks)packages/components/.storybook/main.ts(1 hunks)packages/components/__mocks__/console.ts(1 hunks)packages/components/__mocks__/expo-constants.ts(1 hunks)packages/components/__mocks__/expo-modules-core.ts(1 hunks)packages/components/__mocks__/expo-secure-store.ts(1 hunks)packages/components/__mocks__/expo.ts(0 hunks)packages/components/__mocks__/fetch.ts(1 hunks)packages/components/__mocks__/file.ts(1 hunks)packages/components/__mocks__/fileMock.ts(0 hunks)packages/components/__mocks__/next-font.ts(1 hunks)packages/components/__mocks__/nextFontMock.ts(0 hunks)packages/components/__mocks__/style.ts(1 hunks)packages/components/cypress/__mocks__/nextFontMock.ts(0 hunks)packages/components/jest.config.ts(1 hunks)packages/components/webpack.config.ts(2 hunks)packages/config/relay.config.ts(1 hunks)packages/design-system/.storybook/__mocks__/next-font.ts(1 hunks)packages/design-system/.storybook/__mocks__/nextFontMock.ts(0 hunks)packages/design-system/.storybook/main.ts(1 hunks)packages/design-system/__mocks__/events.ts(1 hunks)packages/design-system/__mocks__/expo-constants.ts(1 hunks)packages/design-system/__mocks__/expo-modules-core.ts(1 hunks)packages/design-system/__mocks__/expo-secure-store.ts(1 hunks)packages/design-system/__mocks__/next-font.ts(1 hunks)packages/design-system/__mocks__/react-native.ts(1 hunks)packages/test/README.md(1 hunks)packages/test/__mocks__/expo-constants.ts(1 hunks)packages/test/__mocks__/expo-modules-core.ts(1 hunks)packages/test/__mocks__/expo-secure-store.ts(1 hunks)packages/test/__mocks__/expo.ts(0 hunks)packages/test/index.tsx(1 hunks)packages/test/jest.config.ts(1 hunks)packages/utils/__mocks__/console.ts(1 hunks)packages/utils/__mocks__/expo-constants.ts(1 hunks)packages/utils/__mocks__/expo-modules-core.ts(1 hunks)packages/utils/__mocks__/expo-secure-store.ts(1 hunks)packages/utils/__mocks__/expo.ts(0 hunks)packages/utils/__mocks__/fetch.ts(1 hunks)packages/utils/__mocks__/file.ts(1 hunks)packages/utils/__mocks__/fileMock.ts(0 hunks)packages/utils/__mocks__/style.ts(1 hunks)packages/wagtail/.storybook/__mocks__/nextFontMock.ts(0 hunks)packages/wagtail/.storybook/main.ts(1 hunks)packages/wagtail/__mocks__/events.ts(1 hunks)packages/wagtail/__mocks__/expo-constants.ts(1 hunks)packages/wagtail/__mocks__/expo-modules-core.ts(1 hunks)packages/wagtail/__mocks__/expo-secure-store.ts(1 hunks)packages/wagtail/__mocks__/next-font.ts(1 hunks)packages/wagtail/__mocks__/react-native.ts(1 hunks)packages/wagtail/webpack.config.ts(2 hunks)
💤 Files with no reviewable changes (12)
- packages/authentication/mocks/expo.ts
- packages/wagtail/.storybook/mocks/nextFontMock.ts
- packages/utils/mocks/fileMock.ts
- packages/components/mocks/expo.ts
- packages/design-system/.storybook/mocks/nextFontMock.ts
- packages/components/.storybook/mocks/nextFontMock.ts
- packages/authentication/mocks/fileMock.ts
- packages/components/cypress/mocks/nextFontMock.ts
- packages/components/mocks/nextFontMock.ts
- packages/utils/mocks/expo.ts
- packages/test/mocks/expo.ts
- packages/components/mocks/fileMock.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/soft-books-sin.md
🚧 Files skipped from review as they are similar to previous changes (46)
- packages/design-system/mocks/next-font.ts
- packages/components/mocks/file.ts
- packages/test/mocks/expo-modules-core.ts
- packages/utils/mocks/expo-secure-store.ts
- packages/design-system/mocks/react-native.ts
- packages/wagtail/mocks/expo-modules-core.ts
- packages/utils/mocks/expo-constants.ts
- packages/wagtail/mocks/expo-constants.ts
- packages/components/mocks/next-font.ts
- packages/design-system/mocks/expo-constants.ts
- packages/design-system/mocks/expo-secure-store.ts
- packages/authentication/mocks/file.ts
- packages/authentication/mocks/expo-constants.ts
- packages/authentication/mocks/expo-secure-store.ts
- packages/components/mocks/style.ts
- packages/wagtail/mocks/next-font.ts
- packages/utils/mocks/expo-modules-core.ts
- packages/test/mocks/expo-constants.ts
- packages/test/index.tsx
- packages/wagtail/mocks/react-native.ts
- packages/authentication/mocks/fetch.ts
- packages/components/mocks/expo-secure-store.ts
- packages/wagtail/mocks/expo-secure-store.ts
- packages/components/mocks/expo-constants.ts
- packages/authentication/mocks/expo-modules-core.ts
- packages/components/mocks/console.ts
- packages/design-system/mocks/expo-modules-core.ts
- packages/authentication/mocks/style.ts
- packages/wagtail/.storybook/main.ts
- packages/components/mocks/expo-modules-core.ts
- packages/components/jest.config.ts
- packages/utils/mocks/file.ts
- packages/utils/mocks/fetch.ts
- packages/config/relay.config.ts
- packages/components/.storybook/main.ts
- packages/test/mocks/expo-secure-store.ts
- packages/authentication/mocks/console.ts
- packages/components/mocks/fetch.ts
- packages/wagtail/webpack.config.ts
- packages/components/webpack.config.ts
- packages/utils/mocks/style.ts
- packages/design-system/.storybook/main.ts
- packages/test/jest.config.ts
- packages/design-system/.storybook/mocks/next-font.ts
- packages/test/README.md
- packages/utils/mocks/console.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/wagtail/__mocks__/events.ts
[error] 2-2: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 4-4: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 17-17: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
packages/design-system/__mocks__/events.ts
[error] 17-17: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
🔇 Additional comments (1)
packages/design-system/__mocks__/events.ts (1)
Line range hint
1-23: Consolidate with the wagtail package implementation.This implementation shares the same code and issues with the wagtail package implementation. As mentioned in the wagtail review, these should be consolidated into a shared location.
The same improvements regarding type safety, error handling, and proper TypeScript typing should be applied to the consolidated version.
🧰 Tools
🪛 Biome (1.9.4)
[error] 17-17: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
| export class EventEmitter { | ||
| private events: Record<string, Function[]> = {} | ||
|
|
||
| on(event: string, listener: Function) { | ||
| if (!this.events[event]) { | ||
| this.events[event] = [] | ||
| } | ||
| this.events[event]?.push(listener) | ||
| } | ||
|
|
||
| emit(event: string, ...args: any[]) { | ||
| if (this.events[event]) { | ||
| this.events[event]?.forEach((listener) => listener(...args)) | ||
| } | ||
| } | ||
|
|
||
| off(event: string, listener: Function) { | ||
| if (this.events[event]) { | ||
| // @ts-ignore | ||
| this.events[event] = this.events[event]?.filter((l) => l !== listener) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate duplicate EventEmitter implementations.
This implementation is duplicated across packages. Consider moving it to a shared location to maintain consistency and reduce duplication.
I can help create a shared implementation that can be imported by both packages. Would you like me to propose a solution?
🧰 Tools
🪛 Biome (1.9.4)
[error] 2-2: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 4-4: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 17-17: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
| // @ts-ignore | ||
| this.events[event] = this.events[event]?.filter((l) => l !== listener) |
There was a problem hiding this comment.
Remove @ts-ignore and handle type safety properly.
Instead of suppressing the TypeScript error, implement proper type checking.
- // @ts-ignore
- this.events[event] = this.events[event]?.filter((l) => l !== listener)
+ const listeners = this.events[event]
+ if (listeners) {
+ this.events[event] = listeners.filter((l) => l !== listener)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // @ts-ignore | |
| this.events[event] = this.events[event]?.filter((l) => l !== listener) | |
| const listeners = this.events[event] | |
| if (listeners) { | |
| this.events[event] = listeners.filter((l) => l !== listener) | |
| } |
anicioalexandre
force-pushed
the
BA-2130-organize-mocks
branch
from
ec0ceb1 to
b7a41f2
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/design-system/__mocks__/events.ts (2)
13-13: 🛠️ Refactor suggestionRemove redundant optional chaining and add error handling.
The optional chaining is unnecessary due to the if-check above. Additionally, the implementation could benefit from error handling.
- this.events[event]?.forEach((listener) => listener(...args)) + this.events[event].forEach((listener) => { + try { + listener(...args) + } catch (error) { + console.error(`Error in event listener for ${event}:`, error) + } + })
19-20: 🛠️ Refactor suggestionImprove type safety and remove redundant code.
Multiple issues to address:
- Optional chaining is redundant due to the if-check
@ts-ignoreis masking potential type issuesFunctiontype usage should be replaced with proper typingFirst, add proper type definition at the top of the file:
type EventListener = (...args: any[]) => voidThen apply these changes:
- private events: Record<string, Function[]> = {} + private events: Record<string, EventListener[]> = {} - on(event: string, listener: Function) { + on(event: string, listener: EventListener) { - off(event: string, listener: Function) { + off(event: string, listener: EventListener) { if (this.events[event]) { - // @ts-ignore - this.events[event] = this.events[event]?.filter((l) => l !== listener) + this.events[event] = this.events[event].filter((l) => l !== listener) } }
🧹 Nitpick comments (1)
packages/design-system/__mocks__/events.ts (1)
8-8: Remove unnecessary optional chaining.The optional chaining operator (
?.) is redundant here since the if-check above ensuresthis.events[event]exists and initializes it if it doesn't.- this.events[event]?.push(listener) + this.events[event].push(listener)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between ec0ceb122f777686ef3319049cda872f30a08c9b and b7a41f2.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
.changeset/config.json(1 hunks)package.json(1 hunks)packages/authentication/CHANGELOG.md(1 hunks)packages/authentication/package.json(1 hunks)packages/components/CHANGELOG.md(1 hunks)packages/components/package.json(1 hunks)packages/config/CHANGELOG.md(1 hunks)packages/config/package.json(1 hunks)packages/design-system/CHANGELOG.md(1 hunks)packages/design-system/__mocks__/events.ts(1 hunks)packages/design-system/package.json(1 hunks)packages/graphql/CHANGELOG.md(1 hunks)packages/graphql/package.json(1 hunks)packages/provider/CHANGELOG.md(1 hunks)packages/provider/package.json(1 hunks)packages/test/CHANGELOG.md(1 hunks)packages/test/package.json(1 hunks)packages/utils/CHANGELOG.md(1 hunks)packages/utils/package.json(1 hunks)packages/wagtail/CHANGELOG.md(1 hunks)packages/wagtail/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (19)
- packages/design-system/package.json
- packages/config/package.json
- packages/config/CHANGELOG.md
- packages/graphql/package.json
- packages/utils/package.json
- packages/wagtail/package.json
- packages/test/package.json
- packages/provider/package.json
- packages/components/package.json
- packages/authentication/package.json
- .changeset/config.json
- packages/graphql/CHANGELOG.md
- packages/provider/CHANGELOG.md
- packages/test/CHANGELOG.md
- packages/wagtail/CHANGELOG.md
- packages/utils/CHANGELOG.md
- packages/authentication/CHANGELOG.md
- packages/design-system/CHANGELOG.md
- packages/components/CHANGELOG.md
🧰 Additional context used
🪛 Biome (1.9.4)
packages/design-system/__mocks__/events.ts
[error] 17-17: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
🔇 Additional comments (1)
package.json (1)
22-22: Verify the changelog for breaking changes in @changesets/cli.The update from
^2.23.0to^2.27.11spans multiple minor versions. While the caret indicates backward compatibility, it's good practice to review the changelog.Let's check the changelog and any potential breaking changes:
✅ Verification successful
✅ Safe to update @changesets/cli - includes security fixes
The update from 2.23.0 to 2.27.11 contains only backward-compatible changes and bug fixes, including an important security fix for a cross-spawn vulnerability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Fetch the changelog for @changesets/cli between versions 2.23.0 and 2.27.11 # Get the release notes from GitHub gh api \ -H "Accept: application/vnd.github+json" \ repos/changesets/changesets/releases \ --jq '.[] | select(.tag_name | startswith("@changesets/cli@")) | select(.tag_name | test("@changesets/cli@2.(2[3-9]|27)")) | {version: .tag_name, body: .body}' # Verify that the scripts using changesets still work echo "Note: Please test the following npm scripts after the update:" echo " - npm run changeset" echo " - npm run version-packages" echo " - npm run release"Length of output: 6500
__package_name__package update -v __package_version__Summary by CodeRabbit
Based on the comprehensive summary of changes, here are the release notes:
Mock File Updates
Dependency Updates
@baseapp-frontend/authentication: 4.1.2 → 4.1.3@baseapp-frontend/components: 0.0.49 → 0.0.50@baseapp-frontend/design-system: 0.0.30 → 0.0.31@baseapp-frontend/graphql: 1.2.2 → 1.2.3@baseapp-frontend/utils: 3.1.1 → 3.1.2@baseapp-frontend/provider: 2.0.8 → 2.0.9@baseapp-frontend/wagtail: 1.0.16 → 1.0.17Tooling
@changesets/clifrom 2.23.0 to 2.27.11