search

DimitarNestorov / react-devtools-electron

React Developer Tools for Electron
https://npm.im/react-devtools-electron
MIT License
7 stars 4 forks source link

Remove old electron support #361

Closed stefansundin closed 9 months ago

stefansundin commented 2 years ago

Hello.

Thanks for the package. However, I was not able to use it directly since I got these TypeScript errors, and I couldn't find a way to ignore them (couldn't get @ts-ignore to do it, and none of the other things that I tried worked either).

17:52:29.448 › TypeError: electron_1.BrowserWindow.getDevToolsExtensions is not a function
    at App.addReactDevToolsExtension (/Users/.../node_modules/react-devtools-electron/dist/utils.js:8:47)
    at Object.onceWrapper (node:events:514:26)
    at App.emit (node:events:406:35)

I suppose that I could perhaps have solved it by coercing the deprecated methods with as any, but since I am on an M1 Mac, I am not able to install any electron version older than v11. That's also why I had to update the submodule to the latest master (even the latest tag failed to build for me).

So hopefully this is acceptable. I would suggest bumping the major version since support for old versions are going away. Thanks!

DimitarNestorov commented 2 years ago

It works for you? I checked out your branch and it doesn't work for me. I have been trying to get it to work for ages on Electron 11+.

stefansundin commented 2 years ago

Yep. With the changes I made here, all I had to do was require('react-devtools-electron'); and it works just fine.

DimitarNestorov commented 2 years ago

Did you publish it on npm? What operating system are you running?

stefansundin commented 2 years ago

Yes, you can try out the fork here: https://www.npmjs.com/package/@spotxyz/react-devtools-electron. I'm using macOS.

stefansundin commented 2 years ago

Hello again,

I might have figured out why you weren't able to get it working on newer electron versions. I assume you've been using the examples in the repository? They load the page from file:// which requires that a new option is used when loading the extension. I was testing the package with my application which loads a page from https://, which explains why it worked fine for me.

https://github.com/electron/electron/commit/a5e9af330f9c819530549a04f0eed9d79dade443

Even though this option was added in electron 13, it is ok to use it on older versions. The additional argument will just be ignored on electron 11 and 12.

I also took the liberty of changing how the extension is loaded. I really want to always include the extension even in my production release, but then only load it if the user is a developer (and also have the ability to unload and load the extension on demand). I updated the README accordingly. This is a breaking change but I think it improves the flexibility of this package by a lot.

I also added examples for electron 17 and 19, and tweaked them as needed. And I updated the extension to the latest version.

Please let me know if you want me to change anything.

DimitarNestorov commented 2 years ago

Interesting. Thanks for the contribution and figuring this out! I will take a look next week and hopefully release some long overdue versions of this package. Should I drop support for Electron 12 and older to keep things simple?

Simon-Laux commented 2 years ago

yes I think you can drop support electron 12 is not supported by electron anymore anyway.

stefansundin commented 2 years ago

Should I drop support for Electron 12 and older to keep things simple?

I don't see a reason to programmatically prevent it from working. I think this will work with Electron 9 and up, but the lowest I can test is Electron 11. But sure, should be totally fine to note an arbitrary version as the lowest supported version (or maybe lowest tested version?) in the README.

stefansundin commented 2 years ago

I just wanted to note that in my attempts to get this to work, I changed some of the yarn workspace stuff. Feel free to revert that if you don't like it. :)

The root of the problem was that typescript kept complaining about mismatching types for electron. When running inside one of the example-electron-* directories, it also pulled in the electron types from the library directory and complained about duplicate types or something. If you get those errors then an easy way to get past it is to delete the library/node_modules/electron directory. I have no idea if there's a typescript option to fix this.

DimitarNestorov commented 2 years ago

Cloned, ran a build, and it worked!

image

I want to look at this PR one more time before merging it. To make things simple I'm planning to just merge as is and then revert whatever I do not agree with. For some reason I thought that the extension didn't work on Electron 11. No need to drop support then.

stefansundin commented 10 months ago

@DimitarNestorov Hello. I'm cleaning up my stale PRs, so I'll close this PR soon if there is no desire to merge it. Thanks!