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

🐛 Removed source-map-support for ts-jest. #1562

Merged
merged 3 commits into from
Jul 3, 2023

Conversation

jBernavaPrah
Copy link

Proposed Changes

Fix: #1561

Types of Changes

  • 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 change)

Checklist

  • My code follows the code style of this project
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

@jBernavaPrah
Copy link
Author

Hi!
This is my first PR here. If I did something wrong let me know :)

Thanks!

@aswetlow
Copy link
Member

aswetlow commented Jun 28, 2023

Hey @jBernavaPrah
Great catch! I had the same issue a few weeks ago but couldn't find a quick fix. I just realized we had a solution for this in v3

require('source-map-support').install(); // tslint:disable-line

Removing the install part would break stuff. But it will work if it is not used in Jest. Could you update your code?

try {
  // do not use source map support with jest.
  if (process.env.JEST_WORKER_ID === undefined) {
    // eslint-disable-next-line @typescript-eslint/no-var-requires
    require('source-map-support').install(); 
  }
} catch (error) {
  Log.error(error);
}

@jBernavaPrah
Copy link
Author

Thanks @aswetlow!

I will do the change you suggested.

I have a concern regarding the try-catch.
I think that is wrong the wrap the code in the try-catch because in any case, as being impossible to install the source-map-support the process needs to fail.

Mainly 3 reasons:

  1. You tell that this is necessary for another part of the project.
  2. Until now, this was already without the try-catch and “worked”.
  3. With only console error, this will not be possible to catch the error in upper statements.

What do you think or what I'm missing here?

@aswetlow
Copy link
Member

Sorry, I did a simple copy&paste from the old version. I can't remember why we added the try/catch block, but I think it failed in some environments. It may not be relevant anymore since we didn't have issues with it in v4.

It's a simple error logging in the catch block because it was okay to fail silently.

You can remove the try/catch block.

if (process.env.JEST_WORKER_ID === undefined) {
    // eslint-disable-next-line @typescript-eslint/no-var-requires
    require('source-map-support').install(); 
  }

Thank you for questioning my suggestion.

@jBernavaPrah
Copy link
Author

Fix done! :)

Thanks for explaining it @aswetlow!

Copy link
Member

@jankoenig jankoenig left a comment

Choose a reason for hiding this comment

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

Thank you @jBernavaPrah! 🎉

@jankoenig jankoenig changed the base branch from v4/latest to v4/dev July 3, 2023 09:59
@jankoenig jankoenig merged commit e704690 into jovotech:v4/dev Jul 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants