Skip to content

fix: prevent undefined from overriding defaults in defineBKTConfig#236

Merged
cre8ivejp merged 16 commits into
mainfrom
fix/undefined-override-bug
Aug 7, 2025
Merged

fix: prevent undefined from overriding defaults in defineBKTConfig#236
cre8ivejp merged 16 commits into
mainfrom
fix/undefined-override-bug

Conversation

@duyhungtnn
Copy link
Copy Markdown
Collaborator

@duyhungtnn duyhungtnn commented Aug 5, 2025

Changes

  • This PR fixed the bug "Undefined Values Override Defaults in defineBKTConfig"
  • Also add a custom lint rule to prevent this issue in the future

Bug Report: Undefined Values Override Defaults in defineBKTConfig

Issue Summary

The defineBKTConfig function has a critical bug where the spread operator ...config can override default values with undefined, causing properties that should have defaults to become undefined and potentially break the application.

It was related to an issue found in the React Native SDK: the SDK is not using InMemoryStorage when AsyncStorage not available

Root Cause

Location: src/BKTConfig.ts, line 88

const result: BKTConfig = {
  featureTag: '',
  eventsFlushInterval: MINIMUM_FLUSH_INTERVAL_MILLIS,
  eventsMaxQueueSize: DEFAULT_MAX_QUEUE_SIZE,
  pollingInterval: DEFAULT_POLLING_INTERVAL_MILLIS,
  storageKeyPrefix: '',
  userAgent,
  fetch,
  storageFactory: createBKTStorage,
  ...config, // BUG: This can override defaults with undefined values
}

The spread operation happens after setting defaults, so any property in config that is explicitly set to undefined will override the intended default value.

Affected Properties

Properties with NO validation (can remain undefined):

  • eventsMaxQueueSize - No check, can be undefined
  • storageKeyPrefix - No check, can be undefined
  • storageFactory - No check, can be undefined

Properties with inadequate validation:

  • pollingInterval - Uses < MINIMUM_POLLING_INTERVAL_MILLIS check, but undefined < number is false, so undefined passes through
  • eventsFlushInterval - Same issue as above

Properties with working validation:

  • featureTag - Has explicit === undefined check
  • userAgent - Has !result.userAgent check
  • fetch - Has !result.fetch check that throws error

How to Reproduce

import { defineBKTConfig } from './src/BKTConfig'

// Example 1: storageFactory becomes undefined
const config1 = {
  apiKey: 'test-key',
  apiEndpoint: 'https://api.example.com',
  appVersion: '1.0.0',
  storageFactory: undefined, // This overrides the default createBKTStorage
}

const result1 = defineBKTConfig(config1)
console.log(result1.storageFactory) // undefined (should be createBKTStorage)

// Example 2: eventsMaxQueueSize becomes undefined
const config2 = {
  apiKey: 'test-key',
  apiEndpoint: 'https://api.example.com',
  appVersion: '1.0.0',
  eventsMaxQueueSize: undefined, // This overrides the default 50
}

const result2 = defineBKTConfig(config2)
console.log(result2.eventsMaxQueueSize) // undefined (should be 50)

// Example 3: pollingInterval becomes undefined (bypasses validation)
const config3 = {
  apiKey: 'test-key',
  apiEndpoint: 'https://api.example.com',
  appVersion: '1.0.0',
  pollingInterval: undefined, // This overrides the default, and undefined < 60000 is false
}

const result3 = defineBKTConfig(config3)
console.log(result3.pollingInterval) // undefined (should be 600000)

// Example 4: Multiple undefined values causing issues
const config4 = {
  apiKey: 'test-key',
  apiEndpoint: 'https://api.example.com',
  appVersion: '1.0.0',
  eventsMaxQueueSize: undefined,
  storageKeyPrefix: undefined,
  pollingInterval: undefined,
  eventsFlushInterval: undefined,
}

const result4 = defineBKTConfig(config4)
// All these properties will be undefined instead of their defaults:
console.log(result4.eventsMaxQueueSize)  // undefined (should be 50)
console.log(result4.storageKeyPrefix)    // undefined (should be '')
console.log(result4.pollingInterval)     // undefined (should be 600000)
console.log(result4.eventsFlushInterval) // undefined (should be 30000)

Impact

  • Runtime errors when undefined properties are used
  • Inconsistent behavior between properties
  • Type safety violation (TypeScript thinks properties are non-nullable, but they can be undefined)
  • Potential crashes when calling methods on undefined objects (e.g., storageFactory)

@duyhungtnn duyhungtnn marked this pull request as draft August 5, 2025 11:16
@duyhungtnn duyhungtnn force-pushed the fix/undefined-override-bug branch from d7e110b to ebceab7 Compare August 5, 2025 11:29
@duyhungtnn duyhungtnn requested a review from Copilot August 5, 2025 14:05

This comment was marked as outdated.

Replaces use of hasOwnProperty with Object.prototype.hasOwnProperty.call to ensure correct property check for 'fetch' in config. This prevents potential issues if config does not inherit directly from Object.
Refines the check for the 'fetch' property to use the 'in' operator and adds an additional validation to ensure 'fetch' is always defined in the final config. This guarantees the SDK always has a fetch function available for network requests.
@duyhungtnn duyhungtnn requested a review from Copilot August 5, 2025 14:44

This comment was marked as outdated.

Removed special handling that threw an error when fetch was explicitly set to undefined. Now, passing undefined for fetch will use the global fetch as a fallback, and the related test has been updated to reflect this behavior.
@duyhungtnn duyhungtnn requested a review from Copilot August 5, 2025 15:09

This comment was marked as outdated.

Added an internal comment clarifying the fetch validation in defineBKTConfig to ensure a fetch implementation is always available. Also fixed a typo in the related test name ('throws' to 'throw').
@duyhungtnn duyhungtnn requested a review from Copilot August 5, 2025 15:13

This comment was marked as outdated.

Enhanced the validation for the fetch property to ensure it is a function. The error message now provides clearer guidance for environments like Node.js where a fetch implementation must be supplied.
@duyhungtnn duyhungtnn requested a review from Copilot August 5, 2025 15:16

This comment was marked as outdated.

Updated the fetch validation logic to explicitly check for the presence of the fetch property before verifying its type. This ensures that missing or undefined fetch implementations are properly handled and a clear error message is provided.
@duyhungtnn duyhungtnn requested a review from Copilot August 5, 2025 15:22

This comment was marked as outdated.

@duyhungtnn duyhungtnn marked this pull request as ready for review August 5, 2025 15:25
@duyhungtnn duyhungtnn requested a review from cre8ivejp August 5, 2025 15:27
Introduces a custom ESLint rule 'no-spread-after-defaults' to disallow spreading objects after default properties in object literals. Updates eslint.config.cjs to include the new rule, expands ignore patterns, and sets parser options. Upgrades @typescript-eslint/eslint-plugin and @typescript-eslint/parser to version 8.39.0. Updates tsconfig.json to enable strictNullChecks and expand included files.
Replaces manual lazy initialization patterns with nullish coalescing assignment (??=) in DI modules and related classes for improved readability and conciseness. Updates affected tests to match new initialization style and applies minor formatting improvements.
Corrected indentation in the visitor function to improve code readability and maintain consistent formatting.
@duyhungtnn duyhungtnn requested a review from Copilot August 6, 2025 03:51

This comment was marked as outdated.

Replaces object spread for advanced config properties in BKTConfig with explicit assignments to avoid leaking undefined values. Updates ESLint rule to flag all spreads after defaults, not just identifiers.
@duyhungtnn duyhungtnn requested a review from Copilot August 6, 2025 04:10

This comment was marked as outdated.

Replaces the default fetch assignment from 'fetch' to 'globalThis.fetch' in BKTConfig. This ensures compatibility with environments where 'fetch' is available globally.
Enhanced the no-spread-after-defaults ESLint rule to better detect default property spreads and added explanatory comments with rule disables in event creator functions. Updated ESLint config to ignore build.config.ts and use projectService, and changed tsconfig.vitest.json to allow emitting files.
Simplified the detection logic for spread elements after default properties in the ESLint rule, treating any spread after a property as a violation. Added a comprehensive documentation file explaining the rule, rationale, examples, alternatives, and configuration.
@duyhungtnn duyhungtnn requested a review from Copilot August 6, 2025 10:14

This comment was marked as outdated.

@duyhungtnn duyhungtnn marked this pull request as draft August 6, 2025 14:03
Added unit tests for the no-spread-after-defaults custom ESLint rule and a new npm script to run them. Updated comments in EventCreators.ts to clarify the safety of spreading fields after base objects due to TypeScript's Omit utility.
Adds a step to run custom ESLint rule tests in the build workflow. Updates the test completion log message in no-spread-after-defaults.test.js for clarity.
@duyhungtnn duyhungtnn requested a review from Copilot August 7, 2025 02:51
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a critical bug in the defineBKTConfig function where undefined values in the configuration object could override default values due to improper use of the spread operator. The fix replaces the problematic spread pattern with explicit nullish coalescing operators and adds a custom ESLint rule to prevent similar issues in the future.

Key Changes:

  • Fixed the defineBKTConfig function to properly handle undefined values using nullish coalescing
  • Added comprehensive test coverage for the undefined value scenarios
  • Implemented a custom ESLint rule no-spread-after-defaults to prevent similar bugs
  • Applied code style improvements throughout the codebase

Reviewed Changes

Copilot reviewed 16 out of 18 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/BKTConfig.ts Main fix: replaced spread operator with explicit property assignment using nullish coalescing
test/BKTConfig.spec.ts Added comprehensive tests for undefined value handling scenarios
eslint-rules/no-spread-after-defaults.js Custom ESLint rule to prevent spread-after-defaults pattern
eslint-rules/no-spread-after-defaults.test.js Test suite for the custom ESLint rule
eslint-rules/no-spread-after-defaults.md Documentation for the custom ESLint rule
eslint.config.cjs ESLint configuration updates to include custom rule and new formatting rules
src/internal/event/EventCreators.ts Added ESLint disable comments for legitimate spread usage
Multiple test/internal files Code style improvements using nullish coalescing and formatting
Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

Comment thread src/BKTConfig.ts
Comment thread eslint.config.cjs
@duyhungtnn duyhungtnn marked this pull request as ready for review August 7, 2025 02:52
Copy link
Copy Markdown
Member

@cre8ivejp cre8ivejp left a comment

Choose a reason for hiding this comment

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

Thank you!

@cre8ivejp cre8ivejp merged commit 356cf86 into main Aug 7, 2025
2 checks passed
@cre8ivejp cre8ivejp deleted the fix/undefined-override-bug branch August 7, 2025 03:24
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.

3 participants