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

Defer to native-fetch whenever ELECTRON_RUN_AS_NODE is set #140

Open
rj-calvin opened this issue Aug 12, 2021 · 7 comments
Open

Defer to native-fetch whenever ELECTRON_RUN_AS_NODE is set #140

rj-calvin opened this issue Aug 12, 2021 · 7 comments
Labels
need/triage Needs initial labeling and prioritization

Comments

@rj-calvin
Copy link

rj-calvin commented Aug 12, 2021

You guys are using electron-fetch whenever the is-electron package detects if it is running in an electron process (particularly, the main process). However, is-electron seems to be returning true even when in a process that has been forked from electron's main process as a node server.

This is currently what I am trying to set up in my own application, however, since you can't access electron from within a forked process, attempting to load electron-fetch results in an error wherein electron claims that it hasn't been installed properly.

The environment variable ELECTRON_RUN_AS_NODE is used to observe when a process is running as a fork from main, so I was thinking an additional condition could be added to defer to native-fetch whenever it is set.

@rj-calvin rj-calvin added the need/triage Needs initial labeling and prioritization label Aug 12, 2021
@welcome
Copy link

welcome bot commented Aug 12, 2021

Thank you for submitting your first issue to this repository! A maintainer will be here shortly to triage and review.
In the meantime, please double-check that you have provided all the necessary information to make this process easy! Any information that can help save additional round trips is useful! We currently aim to give initial feedback within two business days. If this does not happen, feel free to leave a comment.
Please keep an eye on how this issue will be labeled, as labels give an overview of priorities, assignments and additional actions requested by the maintainers:

  • "Priority" labels will show how urgent this is for the team.
  • "Status" labels will show if this is ready to be worked on, blocked, or in progress.
  • "Need" labels will indicate if additional input or analysis is required.

Finally, remember to use https://discuss.ipfs.io if you just need general support.

@rj-calvin rj-calvin changed the title Don't use electron-fetch in a process forked from electron Defer to native-fetch whenever ELECTRON_RUN_AS_NODE is set Aug 12, 2021
@achingbrain
Copy link
Member

This sounds like it should be a PR to is-electron - have you opened an issue there?

@rj-calvin
Copy link
Author

I considered that, but I believe that it's likely that this would be a breaking change there.

rj-calvin added a commit to gatsby-tv/js-ipfs-utils that referenced this issue Aug 14, 2021
rj-calvin added a commit to gatsby-tv/js-ipfs-utils that referenced this issue Aug 14, 2021
@lidel
Copy link
Member

lidel commented Oct 1, 2021

Blocked for now: waiting for Node 16 to become LTS, and maybe even Electron to ship ELECTRON_RUN_AS_NODE with implementation matching Node 16+ changes around node-fetch.

@lidel lidel added the status/blocked Unable to be worked further until needs are met label Oct 1, 2021
@rj-calvin
Copy link
Author

rj-calvin commented Oct 3, 2021

Blocked for now: waiting for Node 16 to become LTS, and maybe even Electron to ship ELECTRON_RUN_AS_NODE with implementation matching Node 16+ changes around node-fetch.

If anyone needs to have this issue fixed in the meantime, I have a branch with a workaround here: https://github.com/gatsby-tv/js-ipfs-utils

One would only need to add the following to their package.json:

{
  "resolutions": {
    "ipfs-utils": "https://github.com/gatsby-tv/js-ipfs-utils#fix/defer-to-native-fetch"
  }
}

@lidel lidel removed the status/blocked Unable to be worked further until needs are met label Nov 26, 2021
@dkuhnert
Copy link

Same issue here. This is also broken in workers.

The electron.net API should only be used when process.type === "browser".
Technically it's still an electron process (as for example process.versions.electron etc. is available) but it's not the main (browser) process. IMHO is-electron is right as it only tells its "some" electron process.

Changing

const IS_ELECTRON_MAIN = IS_ELECTRON && !IS_ENV_WITH_DOM
to

const IS_ELECTRON_MAIN = IS_ELECTRON && !IS_ENV_WITH_DOM && typeof process !== 'undefined' && process.type === "browser"

should do the trick.

@rj-calvin
Copy link
Author

@dkuhnert I don't see why the ELECTRON_RUN_AS_NODE variable wouldn't be sufficient and simpler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/triage Needs initial labeling and prioritization
Projects
No open projects
Development

No branches or pull requests

4 participants