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: import xhr2 to avoid reference error (#101) #102

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tkt9k2562
Copy link

@tkt9k2562 tkt9k2562 commented Dec 23, 2024

Original Issue:
#101

@tkt9k2562
Copy link
Author

I test it manually , by replacing the build/dicomweb-client.js which is generated after npm run build , and it works.

@pieper pieper requested a review from wayfarer3130 December 23, 2024 20:55
@pieper
Copy link
Contributor

pieper commented Dec 23, 2024

What does this do on a browser? Is it transparent and workable in node and browser or do we need to detect?

@wayfarer3130 @sedghi do you have any thoughts on this PR?

@wayfarer3130
Copy link
Contributor

I don't know that it is going to work in a browser - that is something which would have to be tested to see what it does. One could export a method that just choose that import versus the browser version depending on if in node or the browser.

@pieper
Copy link
Contributor

pieper commented Jan 7, 2025

Thanks for the doublecheck @wayfarer3130 .

@tkt9k2562 can you look into making sure this works the same in node and the browser? Ideally with automated testing.

@sedghi
Copy link
Contributor

sedghi commented Jan 7, 2025

I don't think this is the way, not sure what we need from xhr request but we can do something simpler like

let fetch;
if (typeof window === 'undefined') {
    fetch = await import('node-fetch').then(mod => mod.default);
} else {
    fetch = window.fetch; // Use browser's native fetch
}

or the solution above but dynamically choosing/importing that package instead of forcing it at import level

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.

4 participants