electron icon indicating copy to clipboard operation
electron copied to clipboard

Integrate electron plugin

Open gwleuverink opened this issue 1 year ago • 7 comments

Summary

This PR pulls the electron plugin into this repo.

I've landed on the tarball approach @simonhamp proposed in https://github.com/orgs/NativePHP/discussions/348

A couple of changes had to be made in order to make it work. Because a lot of files changed I've listed the steps I took below. I've also commented the changes that might need some more clarification in the diff.

Couple things I did:

  1. pulled in the plugin into resources/js/electron-plugin
  2. some light cleanup, removed duplicate security.md & moved CI script to this repo (needs testing!)
  3. updated package.json to point to the local dependency
  4. updated the native:serve command to do a clean install (so changes in electron-plugin come through)
  5. updated the native:build command to pack the electron plugin as a tarball & do a clean install of the tarball after running npm update. Clean install is needed because the tarball is not versioned, it will not update otherwise.

Note that this packing step is only needed when running native:build.

For your consideration

  1. I noticed I get a lot of linting squigglies in the electron plugin after moving it to this project. Probably a configuration issue. Would you like me to check that out in this PR or separately?

  2. When making changes to the electron-plugin while running build:watch you still need to restart the native:serve command to see your changes. Might be good to add that to the docs https://github.com/NativePHP/laravel/issues/364

  3. When running the native:build & native:serve command the electron plugin is not built, You have to do so beforehand if any changes where made. This makes sense, since a composer install will come with the latest compiled js. But might be confusing for contributors. Maybe something to consider for the docs.

Closing

As before, feel free to disregard or use only as a reference if you're not up for merging. I realise there was no concrete ask to work on this. I was experimenting a little & figured I might as well finish it 😅 If you need me to make changes and I'll do so as soon as I can.

gwleuverink avatar Sep 23 '24 23:09 gwleuverink

I'll need to take a good look through this when I'm back from vacation, but this is really promising! Thanks so much 🙏🏼

simonhamp avatar Sep 24 '24 11:09 simonhamp

I am not that familiar with how NodeJS handles it internally but would a Subpath Import mitigate the need for the whole tarball process? It most likely requires you to change the imports inside the plugin, but does not seem to use symbolic links. Seems to me to be the cleanest way to integrate the plugin without restructuring the entire project.

RobertWesner avatar Sep 28 '24 11:09 RobertWesner

@RobertWesner this is the first time I've come across imports in a package.json file... I think you're right, it may be a much cleaner approach, but it would need some working through.

@gwleuverink have you ever used that before?

simonhamp avatar Sep 30 '24 12:09 simonhamp

To check whether we can use subpath imports in place of the tarball I integrated the plugin here. Was more straightforward than expected and worked mostly out of the box, just had to change the plugin export to ES6. It does run without issue while using native:serve. Yet when I try native:build linux x64 the resulting .AppImage causes this error:

(node:97146) UnhandledPromiseRejectionWarning: Error: spawn /tmp/.mount_Larave5gKNJn/resources/app.asar.unpacked/resources/php/php ENOENT
    at ChildProcess._handle.onexit (node:internal/child_process:286:19)
    at onErrorNT (node:internal/child_process:484:16)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21)
(Use `laravel --trace-warnings ...` to show where the warning was created)
(node:97146) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)

Full log with --trace-warnings:

Error: spawn /tmp/.mount_LaraveMZrpw2/resources/app.asar.unpacked/resources/php/php ENOENT
    at ChildProcess._handle.onexit (node:internal/child_process:286:19)
    at onErrorNT (node:internal/child_process:484:16)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21) {
  errno: -2,
  code: 'ENOENT',
  syscall: 'spawn /tmp/.mount_LaraveMZrpw2/resources/app.asar.unpacked/resources/php/php',
  path: '/tmp/.mount_LaraveMZrpw2/resources/app.asar.unpacked/resources/php/php',
  spawnargs: [ 'artisan', 'native:config' ],
  cmd: '/tmp/.mount_LaraveMZrpw2/resources/app.asar.unpacked/resources/php/php artisan native:config',
  stdout: '',
  stderr: ''
}
Electron API server started on port 4019
Error: spawn /tmp/.mount_LaraveMZrpw2/resources/app.asar.unpacked/resources/php/php ENOENT
    at ChildProcess._handle.onexit (node:internal/child_process:286:19)
    at onErrorNT (node:internal/child_process:484:16)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21) {
  errno: -2,
  code: 'ENOENT',
  syscall: 'spawn /tmp/.mount_LaraveMZrpw2/resources/app.asar.unpacked/resources/php/php',
  path: '/tmp/.mount_LaraveMZrpw2/resources/app.asar.unpacked/resources/php/php',
  spawnargs: [ 'artisan', 'native:php-ini' ],
  cmd: '/tmp/.mount_LaraveMZrpw2/resources/app.asar.unpacked/resources/php/php artisan native:php-ini',
  stdout: '',
  stderr: ''
}
Starting PHP server... /tmp/.mount_LaraveMZrpw2/resources/app.asar.unpacked/resources/php/php artisan serve /tmp/resources/app/ {}
Making sure app folders are available
(node:97265) UnhandledPromiseRejectionWarning: Error: spawn /tmp/.mount_LaraveMZrpw2/resources/app.asar.unpacked/resources/php/php ENOENT
    at ChildProcess._handle.onexit (node:internal/child_process:286:19)
    at onErrorNT (node:internal/child_process:484:16)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21)
    at emitUnhandledRejectionWarning (node:internal/process/promises:298:15)
    at warnWithErrorCodeUnhandledRejectionsMode (node:internal/process/promises:406:5)
    at processPromiseRejections (node:internal/process/promises:470:17)
    at process.processTicksAndRejections (node:internal/process/task_queues:96:32)
(node:97265) Error: spawn /tmp/.mount_LaraveMZrpw2/resources/app.asar.unpacked/resources/php/php ENOENT
    at ChildProcess._handle.onexit (node:internal/child_process:286:19)
    at onErrorNT (node:internal/child_process:484:16)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21)

The confusing part here is resources/php/php exists. Does someone have an idea on what could cause it? I spent around 2h debugging... Any clues on where to look since the issue seems to be with the build process specifically?

@simonhamp, @gwleuverink, got any thoughts on this?

RobertWesner avatar Oct 02 '24 19:10 RobertWesner

@RobertWesner this is the first time I've come across imports in a package.json file... I think you're right, it may be a much cleaner approach, but it would need some working through.

@gwleuverink have you ever used that before?

No this is the first I hear of it. It looks promising 👀 Thanks for pitching in @RobertWesner!

@simonhamp, @gwleuverink, got any thoughts on this?

The error is unfamiliar to me. Do you have a php executable in that path? nativephp-electron/resources/js/resources/php?

gwleuverink avatar Oct 03 '24 07:10 gwleuverink

@gwleuverink yes, the php executable exists and ./php -v works.

RobertWesner avatar Oct 03 '24 12:10 RobertWesner

Finally managed to find the issue, getAppPath() needed to be modified from

let appPath = join(__dirname, '../../../../../resources/app/').replace('app.asar', 'app.asar.unpacked');

to

let appPath = join(__dirname, '../../resources/app/').replace('app.asar', 'app.asar.unpacked');

I opened NativePHP/laravel#109 with my changes.

RobertWesner avatar Oct 05 '24 14:10 RobertWesner

Closing in favour of NativePHP/laravel#109

simonhamp avatar Oct 14 '24 18:10 simonhamp