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

[LOC-5969] Ensure compatibility with Local 9 #81

Merged
merged 4 commits into from
Apr 4, 2024

Conversation

nickcernis
Copy link
Contributor

@nickcernis nickcernis commented Apr 4, 2024

  • Bumps add-on version.
  • Bumps local and local-components versions.
  • Moves @electron/remote from devDependencies to dependencies to ensure the add-on loads in Local 9. (An alternative to a larger refactoring to replace electron/remote with IPC calls, which I think is fair given the timeline and desired goal of making this work with Local 9.)
  • Noticed "Show in Finder" in the context menu doesn't work on macOS, so smuggled in a quick fix for that here.

For https://wpengine.atlassian.net/browse/LOC-5969.

Testing

  • Add-on loads in Local 9.0.
  • Tools → Image Optimizer pane loads.
  • Images can be optimized.
  • Context menu still loads and menu items function in image list after optimization.
  • Optimus Prime is still able to summon all Autobots.
Screenshot 2024-04-04 at 08 53 14 Screenshot 2024-04-04 at 08 53 43 Screenshot 2024-04-04 at 08 54 43 Screenshot 2024-04-04 at 08 51 21 Screenshot 2024-04-04 at 08 52 50

@electron/remote needs to be a direct dependency and not a
devDependency under Local 9 (which upgraded Electron).
Otherwise, the add-on fails to load and this error is visible
in Local's console:

 /Applications/Local Beta.app/Contents/Resources/app.asar/node_modules/
 @sentry/utils/dist/instrument.js:111 Error Loading Add-on: /Users/
 user.name/Library/Application Support/Local Beta/addons/
 local-addon-image-optimizer/lib/renderer.js
 TypeError: i.isDesktopCapturerEnabled is not a function

An alternative is to refactor to replace @electron/remote with
IPC calls, but that is a larger lift with bigger testing impact given
use of remote.Menu, remote.MenuItem, remote.shell, and the
reportAnalytics calls that currently trigger on menu events.
@nickcernis nickcernis requested a review from a team as a code owner April 4, 2024 07:20
@nickcernis nickcernis requested review from bgturner and removed request for bgturner April 4, 2024 07:20
Same fix we applied here due to showItemInFolder flakiness in Electron
under macOS:

https://github.com/getflywheel/flywheel-local/pull/1888/files

Just needed to get the parent folder path so we don't open the
image itself.
@nickcernis nickcernis force-pushed the nc/LOC-5969/local-9-compat branch from ae678bf to 519a55e Compare April 4, 2024 07:28
Copy link
Contributor

@bgturner bgturner left a comment

Choose a reason for hiding this comment

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

Looks good! I QA'd in Local 9.0 Beta and the image-optimization workflow still works!

Thanks for the detailed commit messages describing the errors you encountered and why the commit was necessary!

@bgturner bgturner merged commit 4b04fb7 into master Apr 4, 2024
@nickcernis nickcernis deleted the nc/LOC-5969/local-9-compat branch April 5, 2024 07:17
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