Skip to content

Conversation

@sayomaki
Copy link
Contributor

Description

As per the original comment in the commit 80341a9, the 3 year long issue still holds right now as described by the comment itself. It appears that the compiled version (using yarn build) will append the default key to the imported JSON files, which does not exist and hence giving an error when used by other project (i.e. frontend). However, it does work correctly in testing/jest which appears to use ts-jest instead to transpile the typescript files.

This PR reverts the commit added here: bca2423

Some other possible fixes for this that can be considered (but more testing and discussion required):

  1. Not using a JSON file (could be in a ts file instead, but since it is autogenerated there would be more steps to properly transform this)
  2. Standardising the build procedure to be same as Jest (would be complicated to switch to a different packaging tool entirely)
  3. Making jest use tsc instead of ts-jest (mentioned briefly on the jest docs, but this may affect coverage reporting/results)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Code quality improvements

Checklist

  • I have tested this code

Keep changes for removing commented tests and prettier formatting
@sayomaki sayomaki added Bug Something isn't working important Fixing this is important, but not mission-critical labels Mar 18, 2025
@sayomaki sayomaki requested a review from RichDom2185 March 18, 2025 12:49
@sayomaki sayomaki self-assigned this Mar 18, 2025
@coveralls
Copy link

Pull Request Test Coverage Report for Build 13923617069

Details

  • 13 of 14 (92.86%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 81.097%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/editors/ace/docTooltip/index.ts 13 14 92.86%
Totals Coverage Status
Change from base Build 13757894317: 0.01%
Covered Lines: 10786
Relevant Lines: 12934

💛 - Coveralls

@martin-henz martin-henz added the critical Fixing this is mission-critical label Mar 20, 2025
Copy link
Member

@martin-henz martin-henz left a comment

Choose a reason for hiding this comment

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

Let's merge this. Seems like an urgent fix.

@martin-henz martin-henz merged commit 716c3b9 into master Mar 20, 2025
4 checks passed
@martin-henz martin-henz deleted the fix-json-imports branch March 20, 2025 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something isn't working critical Fixing this is mission-critical important Fixing this is important, but not mission-critical

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants