Skip to content
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

Allow bom markers #168

Merged
merged 3 commits into from
Aug 10, 2023
Merged

Allow bom markers #168

merged 3 commits into from
Aug 10, 2023

Conversation

richfitz
Copy link
Member

@richfitz richfitz commented Aug 9, 2023

A bit of a fight here. There is a package that does this (https://www.npmjs.com/package/strip-bom) but pulling that in requires changing the tsconfig to allow module interop and that breaks some mocks. The code is very simple though so I pulled it in. See nodejs/node-v0.x-archive#1918 for an issue where others have struggled with the underlying issue.

Deployed at https://wodin-dev.dide.ic.ac.uk/infectiousdiseasemodels-2023/apps/ebola/ where it works ok

@codecov
Copy link

codecov bot commented Aug 9, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (77ca1d7) 99.96% compared to head (7705c04) 99.96%.

❗ Current head 7705c04 differs from pull request most recent head a690c30. Consider uploading reports for the commit a690c30 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #168   +/-   ##
=======================================
  Coverage   99.96%   99.96%           
=======================================
  Files         140      140           
  Lines        3131     3131           
  Branches      436      436           
=======================================
  Hits         3130     3130           
  Misses          1        1           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@richfitz richfitz marked this pull request as ready for review August 9, 2023 11:25
Copy link
Contributor

@EmmaLRussell EmmaLRussell left a comment

Choose a reason for hiding this comment

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

Great, not too much drama in the end! Possible change to the mock clearing in the test module to avoid the ordering restriction.

@@ -6,6 +6,12 @@ describe("configReader", () => {
jest.resetAllMocks();
});

it("copes with BOM markers", () => {
// This test needs to go before the mocks below
Copy link
Contributor

Choose a reason for hiding this comment

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

If you change line 6 to jest.restoreAllMocks();. this should no longer be the case!

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks, that does the trick!

@richfitz richfitz merged commit b7a5272 into main Aug 10, 2023
1 check 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.

2 participants