purescript-contrib / purescript-affjax

An asynchronous AJAX library built using Aff.
Apache License 2.0
121 stars 78 forks source link

Electron main process incorrectly detected as browser #161

Closed BeaudanBrown closed 2 years ago

BeaudanBrown commented 3 years ago

I believe the current check to determine if we are in node or browser:

if (typeof module !== "undefined" && module.require && !(typeof process !== "undefined" && process.versions["electron"])) {
    // We are on node.js
....

is flagging both the electron main and renderer processes as browser, causing request calls made from the main process (node) to fail with XMLHttpRequest is not defined

After modifying the check as advised by this comment to the following:

  if (typeof module !== "undefined" && module.require && !(typeof navigator !== "undefined" && /electron/i.test(navigator.userAgent))) {

request calls made from either process work as expected

I don't have a minimum reproducible project so I decided to create an issue rather than a PR in case this is just user error on my end somehow

2jt commented 2 years ago

Hi. I've also found an issue with this condition. A file bundled with parcel for nodejs does not have module.require, so the condition evaluates to false and therefore enters the else branch (browser). After parcel does it jobs, the module object has this shape module.bundle.Module... Why wouldn't a simple condition (something like const isNode = process && process.versions && process.versions.node) work?

aove215 commented 2 years ago

I have the same issue. I described it here:

https://discourse.purescript.org/t/parcel-v2-with-purescript/2666/7

jamesjwood commented 2 years ago

Had the same issue with Parcel, quick fix available at the above fork

nwolverson commented 2 years ago

I used to use affjax successfully in a VSCode extension, but now it is taking the wrong code path (XMLHttpRequest, which doesn't exist, while I do have xhr2 available).

It's doing that for 2 reasons now

I wonder if it's possible to flip the check and check typeof XMLHttpRequest === 'function' to enable that "browser" code path

garyb commented 2 years ago

@jamesjwood has opened #161 with a revised condition, but I like the idea of flipping the check yeah, that seems like a much simpler solution if it works!

I wonder if this kind of require https://github.com/garyb/purescript-debug/blob/144305842dba81169a93b3a3cc75429d5c8389e9/src/Debug.js#L3-L4 would be relevant here too.

nwolverson commented 2 years ago

I think you mean #168. The solution in that PR avoids being confused by module but blacklists both electron renderer & main process via process.versions["electron"]. You could test all of process.versions.node, process.versions["electron"], navigator but it seems to get a bit crazy.

I'm using this https://github.com/nwolverson/purescript-affjax/commit/69427eda42c313e09d2064b620323e1c3e7dd6ac Only tested in-situ in VSCode extension (ie Electron main process) but the test seems to work in node, browser and Electron renderer contexts. Maybe some bundling could screw with it?

(Insult to injury once I got this working my requests still fail because I had some certificate issue and xhr doesn't expose the error message, so it's perhaps not the best in that context)

garyb commented 2 years ago

Oops yes, #168.

I think it should be okay in regards to bundling, since we're not touching the code that actually does the require, just improving the condition for when to take that path, and bundlers won't pay attention to that anyway.

I guess the require thing might be moot when we switch to ES modules, and will need a different solution entirely. Hmm.

JordanMartinez commented 2 years ago

@garyb Can this issue be closed now? Or does electron require its own driver?

garyb commented 2 years ago

I'm not sure actually. I'm pretty sure Electron doesn't have a native XHR object though, and IIRC xhr2 does work, so the node driver is probably sufficient. I'll close for now and we can reopen/create a new issue if we need something additionally.