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

#463@minor: Added partial support for XMLHttpRequest. #520

Merged

Conversation

Mas0nShi
Copy link
Contributor

@Mas0nShi Mas0nShi commented Jun 25, 2022

Copy link
Owner

@capricorn86 capricorn86 left a comment

Choose a reason for hiding this comment

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

Nice job! 🙂 Found a small thing.

packages/happy-dom/src/fetch/ResourceFetchHandler.ts Outdated Show resolved Hide resolved
@Mas0nShi
Copy link
Contributor Author

Mas0nShi commented Jul 1, 2022

Hi, @capricorn86, i need you give me some suggestions about #521 (comment)_

@capricorn86
Copy link
Owner

Hi, @capricorn86, i need you give me some suggestions about #521 (comment)_

Yes I think that is a good idea 👍 Hopefully it works the same way as in the browser.

…) and remove outdated annotation, fixes some typo error.
@Mas0nShi
Copy link
Contributor Author

Hi, @daveed07,
Sorry, I've been too busy in the past few months.
Thank you for your PR!
It's looks like good, but there's still a lot of things we need to correct.

@capricorn86
Copy link
Owner

Hi, @daveed07, Sorry, I've been too busy in the past few months. Thank you for your PR! It's looks like good, but there's still a lot of things we need to correct.

Yes I am planning to fix the last stuff as soon as possible 😊

@Mas0nShi
Copy link
Contributor Author

Mas0nShi commented Oct 22, 2022

Hey, @capricorn86. I think our current priority is to add test units.
But I failed to implement a local HTTP server, can you help me?

@capricorn86
Copy link
Owner

Hey, @capricorn86. I think our current priority is to add test units. But I failed to implement a local HTTP server, can you help me?

Hi! Yes I will try to find some time tonight 🙂

@capricorn86
Copy link
Owner

Hey, @capricorn86. I think our current priority is to add test units. But I failed to implement a local HTTP server, can you help me?

Great work on the implementation. It seems like it is very soon ready 🙂

@Mas0nShi
Copy link
Contributor Author

Mas0nShi commented Oct 27, 2022

Great work on the implementation. It seems like it is very soon ready 🙂

I have a question, when we use a synchronous request and use a child process to get the response, how do we mock it?
All I can think of is getting a real http & https server in local...

@davidortner-ipex
Copy link

Great work on the implementation. It seems like it is very soon ready 🙂

I have a question, when we use a synchronous request and use a child process to get the response, how do we mock it? All I can think of is getting a real http & https server in local...

I started on adding mocking capabilities in setup.js yesterday. Will soon continue on actual tests.

@Mas0nShi
Copy link
Contributor Author

I started on adding mocking capabilities in setup.js yesterday. Will soon continue on actual tests.

okey, I'm looking forward to it. 😄

@capricorn86
Copy link
Owner

@Mas0nShi I have managed to mock the Node.js packages and started to add some unit tests. I will continue as soon as I have some more time.

@kalvenschraut
Copy link
Contributor

Is there any help I can provide to get this across the finish line? Mainly looking for the URL changes so I can do some web worker tests with vitest.

@capricorn86
Copy link
Owner

Is there any help I can provide to get this across the finish line? Mainly looking for the URL changes so I can do some web worker tests with vitest.

Hi @kalvenschraut!

It's a complicated feature, sorry that it takes such a long time.

It is starting to reach the end. It is just some more unit tests that has to be added to make sure that it is stable and will remain so.

@capricorn86 capricorn86 marked this pull request as ready for review December 7, 2022 15:46
@capricorn86 capricorn86 merged commit 89ca67b into capricorn86:master Dec 7, 2022
@capricorn86
Copy link
Owner

@Mas0nShi I felt confident enough to merge this pull request now. Normally I would have liked if you got some more time to review, but many are waiting for it and there are many unit tests covering this. Feel free to review the code and check if something is missing 🙂

@capricorn86
Copy link
Owner

@kalvenschraut it has been merged now 🙂

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.

None yet

5 participants