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

feat: add support for local devtools extensions #259

Conversation

prashantpalikhe
Copy link
Contributor

Implement #237

@prashantpalikhe
Copy link
Contributor Author

Decided to use the BrowserWindow.addDevToolsExtension to keep the changes minimal. Also, the version of electron-devtools-installer that's being used is using the same under the hood.

Moving to session.loadExtension can be a part of another migration dedicated to that. We need to work with extensionId then, which we don't atm.

WDYT?

.replace(/\/$/, '')
.split('/')
.pop();
const isLocalExtension = fs.statSync(extensionId).isDirectory();
Copy link
Collaborator

Choose a reason for hiding this comment

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

@prashantpalikhe Can you please move this logic to the main process? We should try and keep node usage as less as possible in the renderer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. Can you check?

@@ -167,7 +174,9 @@ export default function ExtensionsManager({triggerNavigationReload}) {

<FormHelperText className={styles.extensionsNotice}>
Note: Only DevTool extensions will work properly, other general
browser extensions may not work as intended.
browser extensions may not work as intended. Local devtools
extensions are also supported. Instead of ID, use absolute path to
Copy link
Collaborator

Choose a reason for hiding this comment

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

@prashantpalikhe If possible can you please add an open icon near the help icon and let the user select the folder by using the file explorer?

It will a simple change using the inbuilt dialog API, check out a sample usage here - https://github.com/manojVivek/responsively-app/blob/master/desktop-app/app/menu.js#L176

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. Can you check?

@prashantpalikhe prashantpalikhe force-pushed the feature/implement-support-for-local-devtools-extension branch from d94816a to d4c2d56 Compare July 5, 2020 07:45
@prashantpalikhe prashantpalikhe force-pushed the feature/implement-support-for-local-devtools-extension branch 2 times, most recently from 910b62f to 2f07890 Compare July 5, 2020 08:34
@prashantpalikhe prashantpalikhe force-pushed the feature/implement-support-for-local-devtools-extension branch from 2f07890 to fff4ff9 Compare July 5, 2020 08:36
@manojVivek
Copy link
Collaborator

@prashantpalikhe These looks great, merging it in. Looking forward to more collaboration in the future! 🙌

@manojVivek manojVivek merged commit 99bbdc9 into responsively-org:master Jul 6, 2020
@manojVivek
Copy link
Collaborator

@all-contributors Please add @prashantpalikhe to code.

@allcontributors
Copy link
Contributor

@manojVivek

I've put up a pull request to add @prashantpalikhe! 🎉

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.

2 participants