-
Notifications
You must be signed in to change notification settings - Fork 1.9k
fix(client-core): Isolate internalDayjs instance to prevent global dayjs pollution #10279
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
fix(client-core): Isolate internalDayjs instance to prevent global dayjs pollution #10279
Conversation
…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)
There was a problem hiding this 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'withweekStart: 1for 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.
|
@igorlukanin Could you please review this? |
…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
|
There's one test (
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. |
Fix: Isolate internalDayjs instance to prevent global dayjs pollution
Problem
While
packages/cubejs-client-core/src/time.tsattempts to customize an internal dayjs instance, these options were also being applied to the default dayjs instance, causing unintended side effects.Reproduction:
Expected behavior:
Cube Client's dayjs internal instance options should not be reflected in the default dayjs instance.
Changes Made
'cube-internal-en'withweekStart: 1internalDayjsfrom affecting global dayjs instanceinternalDayjschanged global week startTechnical Details
The previous implementation used
.locale({ ...en, weekStart: 1 })inline, which modified the global dayjs locale configuration. The fix:'cube-internal-en')internalDayjscallsThis ensures complete isolation between the internal Cube dayjs instance and the global dayjs instance.
Testing
Check List
Issue Reference
Closes #10165 - @cubejs-client/core "internalDayjs" config affects all dayjs imports
Version:
@cubejs-client/core@^1.5.4and later