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

Make transport Jest-aware #1181

Merged
merged 2 commits into from Oct 25, 2021
Merged

Make transport Jest-aware #1181

merged 2 commits into from Oct 25, 2021

Conversation

mcollina
Copy link
Member

Fixes #1179

@JaneJeon
Copy link

I’ll try it with this PR, see if my CI still fails

@JaneJeon
Copy link

JaneJeon commented Oct 24, 2021

Beautiful, works perfectly (both locally and on CI)!

Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

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

I really don't want to approve this. It seems more like a bug in another package to me. But I'm not blocking it.

@mcollina
Copy link
Member Author

I really don't want to approve this. It seems more like a bug in another package to me. But I'm not blocking it.

I'm with you, but I do not see any other way to fix it. It's a massive design flaw in Jest and part of the enormous technical debt they push on the rest of the ecosystem.

I do not feel we should spend energy to make a stand against jest. My twitter rants about it are enough. Let's land this and move on :(.

@mcollina
Copy link
Member Author

@jsumners I've found a better fix for this.

@mcollina mcollina merged commit 28f1f2c into master Oct 25, 2021
@mcollina mcollina deleted the jest-support branch October 25, 2021 12:35
@JaneJeon
Copy link

@mcollina oh cool, so my suggestion DID work!

@davidmarkclements
Copy link
Member

davidmarkclements commented Oct 25, 2021

struck out below was written with the old solution in mind, should have checked the new one first. Since it it isn't specific to Jest it's an acceptable workaround for us. However, as a general rule, the points below still stand.

for the record I'm also against doing this.
I think we should revert.
This is a Jest issue, we cannot and should not cover for Jest, doing so just takes away any political will for Jest to fix it.

@jsumners
Copy link
Member

Since it it isn't specific to Jest it's an acceptable workaround for us. However, as a general rule, the points below still stand.

I argue it is still specific to Jest. We have to keep the comment in the code explaining why the import is being done the way it is specifically because of Jest.

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ReferenceError when using transports (pino-pretty) with Jest
4 participants