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

fix: replace debug with node debug #2592

Merged
merged 7 commits into from Feb 26, 2024
Merged

Conversation

Uzlopak
Copy link
Member

@Uzlopak Uzlopak commented Feb 25, 2024

I did not fix the unit test as I could not fix it properly. But lets discuss this.

@Uzlopak Uzlopak changed the base branch from main to beta February 25, 2024 02:28
@Uzlopak Uzlopak requested review from gr2m and mikicho February 25, 2024 02:28
lib/debug.js Outdated Show resolved Hide resolved
@Uzlopak
Copy link
Member Author

Uzlopak commented Feb 25, 2024

Just one remark, which I realized later:

debuglog does not exist on browsers. So if we have the necessity to support browsers, it wont work. But I think the support of browsers got lost few major versions ago?

@mikicho
Copy link
Contributor

mikicho commented Feb 25, 2024

@Uzlopak I don't think we support Browser (I think because we use setImmdeiate for example). But if (when?) we complete this effort, we may want to support browsers as well.

@Uzlopak
Copy link
Member Author

Uzlopak commented Feb 25, 2024

Well, i dont know why for browsers nock should be used, but i wanted to say, why maybe debug was used in the first place.

@mikicho
Copy link
Contributor

mikicho commented Feb 25, 2024

@Uzlopak why not? MSW is very popular in the browser.

@gr2m
Copy link
Member

gr2m commented Feb 25, 2024

I wouldn't worry about browsers for now. When we get there we can use ESM features to give non-nodejs environments a different implementation of debug that works in browsers.

@Uzlopak Uzlopak force-pushed the replace-debug-with-node-debug branch from b58a713 to f5eff86 Compare February 25, 2024 18:39
@Uzlopak
Copy link
Member Author

Uzlopak commented Feb 25, 2024

@mikicho @gr2m
PTAL

@mikicho
Copy link
Contributor

mikicho commented Feb 25, 2024

@Uzlopak What is the problem with the tests?

lib/interceptor.js Outdated Show resolved Hide resolved
@Uzlopak
Copy link
Member Author

Uzlopak commented Feb 25, 2024

added test.

Copy link
Member

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

Changes look great, thank you for adding tests!

Not sure about the ERR_INVALID_ARG_TYPE CI error, I've seen that in other PRs as well 😞

@Uzlopak
Copy link
Member Author

Uzlopak commented Feb 25, 2024

Rerunning the tests and it run through

@gr2m
Copy link
Member

gr2m commented Feb 26, 2024

@mikicho if changes look good to you, let's merge

@mikicho
Copy link
Contributor

mikicho commented Feb 26, 2024

LGTM

@Uzlopak Uzlopak changed the title Replace debug with node debug fix: replace debug with node debug Feb 26, 2024
@Uzlopak Uzlopak merged commit e8044c6 into beta Feb 26, 2024
14 checks passed
@Uzlopak Uzlopak deleted the replace-debug-with-node-debug branch February 26, 2024 10:14
Copy link

🎉 This PR is included in version 14.0.0-beta.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants