-
Notifications
You must be signed in to change notification settings - Fork 722
feat: add run from ASAR button #1695
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
base: main
Are you sure you want to change the base?
feat: add run from ASAR button #1695
Conversation
Hey @codebytere , just checking in. Let me know if there's anything I can do to help with the review. Also tagging @dsanders11 , since you opened this issue and are mentoring the GSoC project I’m applying for (Fiddle Support for Fuses), I would love to hear your thoughts as well. Thanks so much for your time and guidance. I really appreciate it and am excited to incorporate any feedback you might have! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pull request! I won't be able to review all of the PR, unfortunately, due to time constraints, but I thought this one was one of the best GSoC-related PRs I've seen so far this year.
I hope that someone else will get to this soon. There's a flood of GSoC-related pull requests at the moment, which keeps everyone busy. Please know that a good PR like this is very much appreciated. Thank you! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good start but it requires some changes and test coverage. 👍
src/renderer/runner.ts
Outdated
interface runOptions { | ||
runFromAsar: boolean; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interface runOptions { | |
runFromAsar: boolean; | |
} | |
export interface RunOptions { | |
runFromAsar?: boolean; | |
} |
Should start with a capital letter to match existing style, and should be exported since it's part of the public API for this file. Since these are options, runFromAsar
should be optional as more options may be added in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, missed the capital letter there, thanks for catching that!
Exported it and made runFromAsar
optional.
src/renderer/runner.ts
Outdated
if (runFromAsar) { | ||
options.push('runFromAsar'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be added to StartFiddleParams
, and then it will be handled automatically by the ...params
in the call to window.ElectronFiddle.startFiddle
below. This will also require changes in src/main/fiddle-core.ts
to plumb it into the call to runner.spawn
.
@AlokYadavCodes, I've changed this PR to be a draft for the moment, the test failures are related to needing the |
Hey @dsanders11 , Also, I submitted a draft GSoC proposal titled "Electron Fiddle Support for Fuses" on April 2. If you have a moment to share any quick feedback before the deadline tomorrow, I'd really appreciate it. Totally understand if you're busy - thanks! |
@AlokYadavCodes, you'll need to bump the version of
Please send it to [email protected] as that's where we provide draft feedback. 🙂 |
@dsanders11 |
@AlokYadavCodes, the UI around the "Run from ASAR" button needs to be improved from my testing, the dropdown is a bit awkward looking with the focus ring around it (could it look more like the the dropdown for public/private gist?) and the dropdown remains there after I've clicked on it. I've also realized that this line is preventing this from working (due to starting options with Line 370 in d5e133f
|
@dsanders11, fixed the UI to match the dropdown style used for public/private gist selection.
Should we also update the fiddle/src/main/fiddle-core.ts Line 56 in 53057ba
|
runOptions
inRunner.run() method
.runOptions
parameter is defined as follows:Updated UI screenshots:
Closes #1551
This functionality requires an upstream change in
@electron/fiddle-core
.I have already opened a PR to implement the required updates in fiddle-core:
Please review both the PR's and let me know if any changes are needed.