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

Support simple electron support via TARGET_ELECTRON env variable #5498

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

danielmahon
Copy link

I know there are "umpteen" ways to support running CRA inside an electron renderer process but most are very "hacky" or require ejecting/forking. This seems to work just fine and seems like a simple addition, and it should support most of the electron use cases. Thoughts?

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@danielmahon
Copy link
Author

Thoughts on this? Anything I can do to help?

@crubier
Copy link

crubier commented Nov 14, 2018

I like this!

@stale
Copy link

stale bot commented Dec 14, 2018

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added stale and removed stale labels Dec 14, 2018
@stale
Copy link

stale bot commented Jan 13, 2019

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Jan 13, 2019
@Johnz86
Copy link

Johnz86 commented Jan 17, 2019

I like this. Someone needs to resolve the branch conflicts.

@stale stale bot removed the stale label Jan 17, 2019
@hardywu
Copy link

hardywu commented Jan 29, 2019

like to have this feature

@stale
Copy link

stale bot commented Feb 28, 2019

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Feb 28, 2019
@tsemerad
Copy link

Since this just got automatically set to 'stale', I just wanted to chime in that this would help me out quite a bit. Is there any interest by the CRA maintainers in having this functionality?

@stale stale bot removed the stale label Feb 28, 2019
@iansu
Copy link
Contributor

iansu commented Mar 1, 2019

We're not necessarily opposed to supporting Electron (as long as the changes are minimal) but it's also not a high priority and we don't really have any maintainers who are Electron experts. So there isn't really anyone driving this and we don't currently have the bandwidth to take it on ourselves.

@danielmahon
Copy link
Author

I need to update this PR to reflect the latest CRA but it was a SUPER simple 2 line change to allow the Webpack target to be configured for the electron renderer which is already supported. So it’s completely opt-in and won’t affect anything else. I’m sure there is more that could be done but in my initial use case simply changing the target was enough to make electron play nicely.

@crubier
Copy link

crubier commented Mar 1, 2019

I may add that this is needed by a lot of people.

@stale
Copy link

stale bot commented Apr 23, 2019

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Apr 23, 2019
@crubier
Copy link

crubier commented Apr 24, 2019

Up! Don't close please

@stale stale bot removed the stale label Apr 24, 2019
@danielmahon danielmahon requested a review from Timer as a code owner June 18, 2019 23:20
@danielmahon danielmahon force-pushed the master branch 2 times, most recently from 5366d04 to 1737dd3 Compare June 18, 2019 23:28
@danielmahon danielmahon changed the title Support simple electron support via ELECTRON env variable Support simple electron support via TARGET_ELECTRON env variable Jun 18, 2019
@iansu iansu added this to In progress in v3.1 via automation Jun 19, 2019
@iansu iansu added this to the 3.1 milestone Jun 19, 2019
@bugzpodder bugzpodder modified the milestones: 3.1, 3.x Jun 19, 2019
@danielmahon
Copy link
Author

Until this lands in 3.1 I've published react-scripts@3.0.1 with this change at https://www.npmjs.com/package/@danielmahon/react-scripts

@bugzpodder
Copy link

I reviewed this PR on technical merits (ie it worked for me). Whether we will start supporting this is still in the air (possibly reviewed in 3.1 milestone). Personally I'd want to see template support before landing this.

@alanzhaonys
Copy link

When are we releasing this to production?

@bugzpodder
Copy link

Copy link
Contributor

@Timer Timer left a comment

Choose a reason for hiding this comment

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

IMO, there should exist a community-maintained fork for Electron apps. We can even link/suggest it in our docs.

Adding direct support for electron increases our support matrix too much and probably won't get the dedicated attention it deserves.

@bugzpodder bugzpodder modified the milestones: 3.x, 100.0 Jul 29, 2019
@bugzpodder bugzpodder removed this from In progress in v3.1 Jul 29, 2019
@bugzpodder bugzpodder removed this from the 100.0 milestone Jul 29, 2019
@alanzhaonys
Copy link

IMO, there should exist a community-maintained fork for Electron apps. We can even link/suggest it in our docs.

Adding direct support for electron increases our support matrix too much and probably won't get the dedicated attention it deserves.

IMO, this should not open up the support for electron, but adding the flexibilities for certain users to extend usage of CRA. CRA is a great tool for people who like me that build Kiosk apps. There is not much replacement out there. There are a lot possibilities that you can do with CRA beyond static contents.

@jlarmstrongiv
Copy link

CRA 3.1 was recently released. Can we put this PR on the roadmap for 3.2?

@lukasfrank
Copy link

lukasfrank commented Aug 26, 2019

CRA 3.1 was recently released. Can we put this PR on the roadmap for 3.2?

Is there actually any roadmap or expected date when the PR get merged? @bugzpodder

@bugzpodder
Copy link

This is off the roadmap for now due to concerns about supporting electron apps after the fact. You can fork this project or rewire the configs if you need it today.

@cinderblock
Copy link

I'd really like to be able to target Electron without the very hacky workarounds.

What if, instead of TARGET_ELECTRON=true, which implies support for Electron (and locks this to Electron) it was something more generic and implicitly cautionary in its naming?

I'm thinking: OVERRIDE_WEBPACK_DEFAULT_TARGET.

It would probably also be prudent to decouple from the Node function mocks.

So maybe OVERRIDE_WEBPACK_DEFAULT_TARGET=electron-renderer DISABLE_NODE_MOCKS=true.

I might also suggest to not fallback on 'web' but instead undefined (or to not set target in settings at all) and to let Webpack use its own default.

@codepunkt
Copy link

This is off the roadmap for now due to concerns about supporting electron apps after the fact. You can fork this project or rewire the configs if you need it today.

What concerns?

@amyrlam amyrlam removed their request for review August 24, 2020 05:34
@ghost
Copy link

ghost commented Dec 3, 2020

Setting the webpack target to electron-renderer is not perfect. When setting to electron-renderer, webpack is assuming that you have nodeIntegration set to true, which is somehow insecure. Instead, most electron apps use the contextBridge API.

Supporting electron is not that easy, however, I have upvoted to merge this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet