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.

Sign up for GitHub

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

Jump to bottom

fix: set "X-Request-Id" header in XMLHttpRequest only in Node.js #397

Merged
merged 7 commits into from
Aug 28, 2023
Merged

fix: set "X-Request-Id" header in XMLHttpRequest only in Node.js #397

merged 7 commits into from
Aug 28, 2023

Conversation

avivasyuta
Copy link
Contributor

@avivasyuta avivasyuta commented Aug 4, 2023 •
edited by kettanaito
Loading

Set the X-Request-Id request header in XMLHttpRequestController only for Node.js env.

@avivasyuta avivasyuta mentioned this pull request Aug 4, 2023
Open
kettanaito
kettanaito requested changes Aug 5, 2023
View reviewed changes
Copy link
Member

@kettanaito kettanaito left a comment

Choose a reason for hiding this comment

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

Thanks for opening this one! I left a comment about determining the browser process more reliably. Let's also see how this affects the existing tests, I hope we're accounting for all the possibilities here.

@avivasyuta
Copy link
Contributor Author

@kettanaito Hello. Could you please approve workflow and review changes?

@avivasyuta avivasyuta mentioned this pull request Aug 20, 2023
Closed
@avivasyuta avivasyuta closed this Aug 21, 2023
@avivasyuta avivasyuta reopened this Aug 21, 2023
@avivasyuta
Copy link
Contributor Author

avivasyuta commented Aug 21, 2023 •
edited
Loading

@kettanaito @msutkowski @marcosvega91 @timdeschryver could you please review this PR?
For more than two weeks, a simple fix has been hanging without any reaction or response.

@kettanaito kettanaito changed the title fix: set x-request-id header in XMLHttpRequestController only for nod… fix: set "X-Request-Id" header in XMLHttpRequest only in Node.js Aug 26, 2023
@kettanaito
Copy link
Member

@avivasyuta, please be respectful of the maintainers' time. All of us approach open source to the best of our abilities, and if no one has looked at this pull request it simply means no one had time. I ask of you to be patient.

@kettanaito
Copy link
Member

I've added a check that we don't send x-request-id during the XHR browser tests. Passes locally, waiting for the CI.

kettanaito
kettanaito approved these changes Aug 26, 2023
View reviewed changes
Copy link
Member

@kettanaito kettanaito left a comment

Choose a reason for hiding this comment

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

This looks good 🚀

@kettanaito
Copy link
Member

Got some weird behavior from Playwright locally vs remote (encourages to uninstall playwright locally but fails without it remotely). Oh well, adding back playwright.

@kettanaito
Copy link
Member

I've resolved the issue with Playwright (see #407). Will update this branch and the tests should pass then.

@kettanaito kettanaito merged commit 0adedc5 into mswjs:main Aug 28, 2023
1 check passed
@kettanaito
Copy link
Member

Welcome to contributors, @avivasyuta 👏

JesusTheHun pushed a commit to JesusTheHun/interceptors that referenced this pull request Aug 29, 2023
@avivasyuta @kettanaito @JesusTheHun
fix: set "X-Request-Id" header in XMLHttpRequest only in Node.js (msw…
94a2a59 
…js#397)

Co-authored-by: avivasyuta <avivasyuta@avito.ru>
Co-authored-by: Artem Zakharchenko <kettanaito@gmail.com>
@avivasyuta
Copy link
Contributor Author

Welcome to contributors, @avivasyuta 👏

Thank you for taking the time to view this PR.

@kettanaito
Copy link
Member

kettanaito commented Aug 29, 2023 •
edited
Loading

This change will be automatically released. But we also need to backport it to 0.17 to the current consumers of MSW would get it as well. @avivasyuta, would you be interested in cherry-picking this change to the backports/0.17 branch?

@kettanaito
Copy link
Member

Released: v0.24.0 🎉

This has been released in v0.24.0!

Make sure to always update to the latest version (npm i @mswjs/interceptors@latest) to get the newest features and bug fixes.

Predictable release automation by @ossjs/release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kettanaito kettanaito kettanaito approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@avivasyuta @kettanaito