Skip to content

fix: import xhr2 to avoid reference error (#101)#102

Open
tkt9k2562 wants to merge 1 commit into
dcmjs-org:masterfrom
tkt9k2562:#101
Open

fix: import xhr2 to avoid reference error (#101)#102
tkt9k2562 wants to merge 1 commit into
dcmjs-org:masterfrom
tkt9k2562:#101

Conversation

@tkt9k2562

@tkt9k2562 tkt9k2562 commented Dec 23, 2024

Copy link
Copy Markdown

Original Issue:
#101

@tkt9k2562

Copy link
Copy Markdown
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

pieper commented Dec 23, 2024

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

pieper commented Jan 7, 2025

Copy link
Copy Markdown
Contributor

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

sedghi commented Jan 7, 2025

Copy link
Copy Markdown
Contributor

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

@wayfarer3130

Copy link
Copy Markdown
Contributor

I'd suggest closing this issue no fix and just using one of the more complete nodejs browser emulators such as the one that jest uses. That way it works in a nodejs and in a browser environment. Also, I think nodejs 24 now has the XMLHttpRequest natively.

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