Skip to content

Conversation

@mystichronicle
Copy link
Contributor

@mystichronicle mystichronicle commented Dec 25, 2025

Fix: Isolate internalDayjs instance to prevent global dayjs pollution

Problem

While packages/cubejs-client-core/src/time.ts attempts to customize an internal dayjs instance, these options were also being applied to the default dayjs instance, causing unintended side effects.

Reproduction:

import dayjs from 'dayjs';
import { internalDayjs } from '@cubejs-client/core'

console.log(dayjs().startOf('week').format('dddd')); // Sunday

const cubeDayjs = internalDayjs(); // sets weekStart to 1

console.log(dayjs().startOf('week').format('dddd')); // Monday ❌

Expected behavior:
Cube Client's dayjs internal instance options should not be reflected in the default dayjs instance.

Changes Made

  • Register custom locale 'cube-internal-en' with weekStart: 1
  • Restore original global locale after registration
  • Prevents internalDayjs from affecting global dayjs instance
  • Add comprehensive test suite for dayjs instance isolation
  • Fixes issue where calling internalDayjs changed global week start

Technical Details

The previous implementation used .locale({ ...en, weekStart: 1 }) inline, which modified the global dayjs locale configuration. The fix:

  1. Creates and registers a custom locale with a unique name ('cube-internal-en')
  2. Captures the current global locale before registration
  3. Registers the custom locale (temporarily changing global locale)
  4. Immediately restores the original global locale
  5. Uses the custom locale name in internalDayjs calls

This ensures complete isolation between the internal Cube dayjs instance and the global dayjs instance.

Testing

  • All 126 existing tests pass
  • Added 4 new comprehensive tests for dayjs instance isolation
  • Manual verification confirms global dayjs is unaffected
  • Coverage maintained for time.ts module

Check List

  • Tests have been run in packages where changes have been made if available
  • Linter has been run for changed code
  • Tests for the changes have been added if not covered yet
  • Docs have been added / updated if required

Issue Reference

Closes #10165 - @cubejs-client/core "internalDayjs" config affects all dayjs imports

Version: @cubejs-client/core@^1.5.4 and later

…yjs pollution

- Register custom locale 'cube-internal-en' with weekStart: 1
- Restore original global locale after registration
- Prevents internalDayjs from affecting global dayjs instance
- Add comprehensive test suite for dayjs instance isolation
- Fixes issue where calling internalDayjs changed global week start

Closes issue where customizing internal dayjs instance affected
the default dayjs instance behavior (weekStart changed from 0 to 1)
Copilot AI review requested due to automatic review settings December 25, 2025 14:42
@mystichronicle mystichronicle requested a review from a team as a code owner December 25, 2025 14:42
@github-actions github-actions bot added client:core Issues relating to the JavaScript client SDK javascript Pull requests that update Javascript code pr:community Contribution from Cube.js community members. labels Dec 25, 2025
Copy link

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 an issue where the internal dayjs instance configuration in @cubejs-client/core was polluting the global dayjs instance. The fix creates a custom locale with a unique name and ensures proper isolation between the internal Cube dayjs instance and the global dayjs instance.

  • Creates a custom locale 'cube-internal-en' with weekStart: 1 for internal use
  • Preserves and restores the global dayjs locale after custom locale registration
  • Adds comprehensive test suite to verify dayjs instance isolation

Reviewed changes

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

File Description
packages/cubejs-client-core/src/time.ts Implements custom locale registration with proper global locale restoration to prevent pollution of the global dayjs instance
packages/cubejs-client-core/test/dayjs-isolation.test.ts Adds new test suite with 4 tests to verify that internalDayjs doesn't affect the global dayjs instance

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@mystichronicle
Copy link
Contributor Author

@igorlukanin Could you please review this?

Debjit Mandal added 2 commits December 25, 2025 20:36
…ects

- Use dayjs.Ls to register locale directly without switching
- Eliminates any potential race conditions during module initialization
- Ensures zero impact on global dayjs instance throughout lifecycle
@mystichronicle
Copy link
Contributor Author

There's one test ("Rolling Mixed With Dimension") that's still failing, but I'm pretty confident it's not related to this fix. Here's why:

  • All the client-side tests pass perfectly (126/126 ✓)
  • The fix works exactly as expected - dayjs instances are properly isolated
  • That failing test is running against the backend server, which uses moment-timezone not dayjs, so our changes shouldn't affect it at all

It looks like either a stale build cache or a snapshot that needs updating. Happy to investigate further if needed, but the core issue this PR addresses is definitely resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

client:core Issues relating to the JavaScript client SDK javascript Pull requests that update Javascript code pr:community Contribution from Cube.js community members.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

@cubejs-client/core "internalDayjs" config affects all dayjs imports

1 participant