Skip to content

Conversation

@Zaimwa9
Copy link
Contributor

@Zaimwa9 Zaimwa9 commented Dec 1, 2025

  • Jsonpath library only exports a default object in ESM mode, meaning import * as jsonpath couldn't access submethods like jsonpath.query
  • All the jsonpath evaluation would throw a silent error and not evaluate
  • This wasn't caught by the tests as they are running in a commonJS context => automatically handles it

Details of the difference between the two systems:

⏺ CommonJS (CJS) - Node.js original module system:
  // Exporting
  module.exports = { query: function() {} }

  // Importing
  const jsonpath = require('jsonpath')
  jsonpath.query()  // ✓ works

  ES Modules (ESM) - Modern JavaScript standard:
  // Exporting
  export default { query: function() {} }
  export const query = function() {}

  // Importing
  import jsonpath from 'jsonpath'      // gets default export
  import * as jsonpath from 'jsonpath' // gets namespace object

  The problem: When a CJS library (like jsonpath) is imported in ESM:
  - import * as jsonpath creates a namespace object where the CJS module.exports becomes jsonpath.default
  - So jsonpath.query is undefined, but jsonpath.default.query works

@Zaimwa9 Zaimwa9 requested a review from a team as a code owner December 1, 2025 15:35
@Zaimwa9 Zaimwa9 requested review from emyller and khvn26 and removed request for a team and emyller December 1, 2025 15:35
khvn26
khvn26 previously approved these changes Dec 1, 2025
Copy link
Member

@khvn26 khvn26 left a comment

Choose a reason for hiding this comment

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

Approving, but I strongly suggest adding a test run in ESM mode to CI, if possible.

@Zaimwa9
Copy link
Contributor Author

Zaimwa9 commented Dec 1, 2025

@khvn26 https://github.com/Flagsmith/flagsmith-nodejs-client/actions/runs/19830086799/job/56813609828?pr=231

New step Run npm run test:esm-build that works on esm

      - run: npm run test:esm-build
        env:
          CI: true

Tests relying on mocks have been skipped due to incompatibilities with vi.mock. (Mostly the percentage_split, version one).

The previous tests are still running so this step is mostly to catch similar issues (that would have been caught)

FAIL  tests/engine/unit/engine.test.ts > test_get_evaluation_result_with_identity_override_and_no_segment_override
AssertionError: expected false to be true // Object.is equality

- Expected
+ Received

- true
+ false

 ❯ tests/engine/unit/engine.test.ts:57:30
     55|         const expected = flag.name === 'overridden_feature' ? true : environmentFeat…
     56| 
     57|         expect(flag.enabled).toBe(expected);
       |                              ^
     58|         expect(flag.reason).toBe(
     59|             flag.name === 'overridden_feature'

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[7/18]⎯

 FAIL  tests/engine/unit/engine.test.ts > evaluateSegments handles segments with identity identifier matching
AssertionError: expected [] to have a length of 2 but got +0

- Expected
+ Received

- 2
+ 0

 ❯ tests/engine/unit/engine.test.ts:195:29
    193|     const result = evaluateSegments(context as any);
    194| 
    195|     expect(result.segments).toHaveLength(2);
       |                             ^
    196|     expect(result.segments).toEqual(
    197|         expect.arrayContaining([

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[8/18]⎯

 FAIL  tests/engine/unit/engine.test.ts > evaluateSegments handles priority conflicts correctly
AssertionError: expected [] to have a length of 2 but got +0

- Expected
+ Received

- 2
+ 0

 ❯ tests/engine/unit/engine.test.ts:282:29
    280|     const result = evaluateSegments(context as any);
    281| 
    282|     expect(result.segments).toHaveLength(2);
       |                             ^
    283| 
    284|     expect(result.segmentOverrides.feature1.segmentName).toBe('high_priority_segment…

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[9/18]⎯

 FAIL  tests/engine/unit/segments/segment_evaluators.test.ts > IN operator > evaluates IN condition {"property":"undefined","operator":"IN","value":["test","john-doe"]} to true
 FAIL  tests/engine/unit/segments/segment_evaluators.test.ts > IN operator > evaluates IN condition {"property":"undefined","operator":"IN","value":"[\"test\", \"john-doe\"]"} to true
 FAIL  tests/engine/unit/segments/segment_evaluators.test.ts > IN operator > evaluates IN condition {"property":"undefined","operator":"IN","value":"test,john-doe"} to true
AssertionError: expected false to be true // Object.is equality

- Expected
+ Received

- true
+ false

 ❯ tests/engine/unit/segments/segment_evaluators.test.ts:244:28
    242|         (condition: SegmentCondition | InSegmentCondition, expected: boolean) => {
    243|             const result = traitsMatchSegmentCondition(condition, 'segment', mockCon…
    244|             expect(result).toBe(expected);
       |                            ^
    245|         }
    246|     );

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[10/18]⎯

 FAIL  tests/engine/unit/segments/segment_evaluators.test.ts > getIdentitySegments single segment evaluation > returns segment when all rules match
AssertionError: expected [] to have a length of 1 but got +0

- Expected
+ Received

- 1
+ 0

 ❯ tests/engine/unit/segments/segment_evaluators.test.ts:310:24
    308| 
    309|         const result = getIdentitySegments(context);
    310|         expect(result).toHaveLength(1);
       |                        ^
    311|         expect(result[0].name).toBe('matching_segment');
    312|     });

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[11/18]⎯

 FAIL  tests/engine/unit/segments/segment_evaluators.test.ts > getContextValue > returns correct value for path $.identity.identifier
AssertionError: expected undefined to be 'user@example.com' // Object.is equality

- Expected: 
"user@example.com"

+ Received: 
undefined

 ❯ tests/engine/unit/segments/segment_evaluators.test.ts:369:24
    367|     ])('returns correct value for path %s', (jsonPath, expected) => {
    368|         const result = getContextValue(jsonPath, mockContext);
    369|         expect(result).toBe(expected);
       |                        ^
    370|     });
    371| 

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[12/18]⎯

 FAIL  tests/engine/unit/segments/segment_evaluators.test.ts > getContextValue > returns correct value for path $.environment.name
AssertionError: expected undefined to be 'Test Environment' // Object.is equality

- Expected: 
"Test Environment"

+ Received: 
undefined

 ❯ tests/engine/unit/segments/segment_evaluators.test.ts:369:24
    367|     ])('returns correct value for path %s', (jsonPath, expected) => {
    368|         const result = getContextValue(jsonPath, mockContext);
    369|         expect(result).toBe(expected);
       |                        ^
    370|     });
    371| 

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[13/18]⎯

 FAIL  tests/engine/unit/segments/segment_evaluators.test.ts > getContextValue > returns correct value for path $.environment.key
AssertionError: expected undefined to be 'test-env-key' // Object.is equality

- Expected: 
"test-env-key"

+ Received: 
undefined

 ❯ tests/engine/unit/segments/segment_evaluators.test.ts:369:24
    367|     ])('returns correct value for path %s', (jsonPath, expected) => {
    368|         const result = getContextValue(jsonPath, mockContext);
    369|         expect(result).toBe(expected);
       |                        ^
    370|     });
    371| 

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯

@Zaimwa9 Zaimwa9 requested a review from khvn26 December 1, 2025 16:51
Copy link
Member

@khvn26 khvn26 left a comment

Choose a reason for hiding this comment

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

Looks solid to me — will appreciate having an issue, or multiple issues, to fix the newly found problematic tests later.

@Zaimwa9 Zaimwa9 merged commit 7a8d949 into main Dec 2, 2025
3 checks passed
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