Integrate electron plugin #108

gwleuverink · 2024-09-23T23:28:43Z

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:

pulled in the plugin into resources/js/electron-plugin
some light cleanup, removed duplicate security.md & moved CI script to this repo (needs testing!)
updated package.json to point to the local dependency
updated the native:serve command to do a clean install (so changes in electron-plugin come through)
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

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?
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 Docs laravel#364
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 · 2024-09-23T23:29:59Z

src/Commands/BuildCommand.php

+        Process::path(__DIR__.'/../../resources/js')
+            ->env($this->getEnvironmentVariables())
+            ->forever()
+            ->run('npm ci ./electron-plugin/nativephp-electron-plugin-tarball.tgz', function (string $type, string $output) {


We need to do a clean install here, because the tarball version was locked. It won't update when changes where made to the electron-plugin otherwise

gwleuverink · 2024-09-23T23:34:33Z

src/Traits/ExecuteCommand.php

@@ -45,7 +45,7 @@ protected function getCommandArrays(string $type = 'install'): array
    {
        $commands = [
            'install' => [
-                'npm' => 'npm install',
+                'npm' => 'npm ci',


We need to do a clean install here as well. Otherwise changes to electron-plugin wont come through.

For example when running npm run build:watch from the plugin directory & running artisan native:serve in a different terminal session.

Having to clean install all dependencies might slow things down a little bit. This is a tradeoff but it could be that the cache negates this. Did not benchmark this yet

It would be great if we didn't have to ci but could rather just force the electron-plugin to be reinstalled from scratch...

gwleuverink · 2024-09-23T23:40:09Z

resources/js/electron-plugin/package.json

@@ -0,0 +1,65 @@
+{
+  "name": "@nativephp/electron-plugin",
+  "version": "tarball",


This might seem weird. I removed the version at first, but npm pack requires a package name and version.
the created taball then includes that version in the filename: nativephp-electron-plugin-tarball.tgz.

In our case we need the tarball to have the same name, hence this string. The filename of the tarball is referenced in the Build command here:

electron/src/Commands/BuildCommand.php

Line 46 in 761d10e

->run('npm ci ./electron-plugin/nativephp-electron-plugin-tarball.tgz', function (string $type, string $output) {

If we randomised the version at runtime and injected this into the package.json, would that negate the need for ci?

Does the version need to be anything specific (incrementing/increasing?) or can it just be random?

I believe it can be random, but we'll have to store the latest version name so we can refer to it in the build script by name.

Can't think of any benefits for that approach from the top of my head. The end result will be the same with some extra overhead. Unless I'm missing something.

In the build script we're only doing a clean install of the plugin. The rest of the dependencies are updated as normal

The serve command does do a clean install for everything. I'm not super happy with that yet.

No, that feels very wasteful

simonhamp · 2024-09-24T11:05:29Z

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

RobertWesner · 2024-09-28T11:50:30Z

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.

simonhamp · 2024-09-30T12:07:10Z

@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?

RobertWesner · 2024-10-02T19:51:55Z

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?

gwleuverink · 2024-10-03T07:45:12Z

@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?

RobertWesner · 2024-10-03T12:50:11Z

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

RobertWesner · 2024-10-05T14:30:50Z

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 #109 with my changes.

simonhamp · 2024-10-14T18:28:22Z

Closing in favour of #109

gwleuverink added 10 commits September 18, 2024 23:11

copy electron-plugin

cd123e7

use install-links to link plugin

d20290b

remove duplicate SECURITY.md

dbd71fa

update readme

63d6821

copy plugin testing workflow

e26e592

remove plugin version

5ff098b

Merge branch 'main' of github.com:gwleuverink/nativephp-electron

2237085

add install-links to npmrc

40d0fc6

remove redundant package version

d3d2eb1

settle for npm-pack strategy

761d10e

gwleuverink commented Sep 23, 2024

View reviewed changes

remove stray .npmrc file

0856216

gwleuverink commented Sep 23, 2024

View reviewed changes

gwleuverink changed the title ~~[RFC-348] Integrate electron plugin~~ Integrate electron plugin Sep 24, 2024

gwleuverink marked this pull request as ready for review September 24, 2024 07:03

RobertWesner mentioned this pull request Oct 5, 2024

Integrate Plugin #109

Merged

simonhamp closed this Oct 14, 2024

gwleuverink deleted the integrate-electron-plugin branch October 14, 2024 19:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate electron plugin #108

Integrate electron plugin #108

gwleuverink commented Sep 23, 2024 •

edited

Loading

gwleuverink Sep 23, 2024

gwleuverink Sep 23, 2024

simonhamp Sep 24, 2024

gwleuverink Sep 23, 2024

simonhamp Sep 24, 2024

gwleuverink Sep 24, 2024

gwleuverink Sep 24, 2024 •

edited

Loading

simonhamp Sep 30, 2024

simonhamp commented Sep 24, 2024

RobertWesner commented Sep 28, 2024

simonhamp commented Sep 30, 2024

RobertWesner commented Oct 2, 2024 •

edited

Loading

gwleuverink commented Oct 3, 2024

RobertWesner commented Oct 3, 2024 •

edited

Loading

RobertWesner commented Oct 5, 2024

simonhamp commented Oct 14, 2024

Integrate electron plugin #108

Integrate electron plugin #108

Conversation

gwleuverink commented Sep 23, 2024 • edited Loading

Summary

For your consideration

Closing

gwleuverink Sep 23, 2024

Choose a reason for hiding this comment

gwleuverink Sep 23, 2024

Choose a reason for hiding this comment

simonhamp Sep 24, 2024

Choose a reason for hiding this comment

gwleuverink Sep 23, 2024

Choose a reason for hiding this comment

simonhamp Sep 24, 2024

Choose a reason for hiding this comment

gwleuverink Sep 24, 2024

Choose a reason for hiding this comment

gwleuverink Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

simonhamp Sep 30, 2024

Choose a reason for hiding this comment

simonhamp commented Sep 24, 2024

RobertWesner commented Sep 28, 2024

simonhamp commented Sep 30, 2024

RobertWesner commented Oct 2, 2024 • edited Loading

gwleuverink commented Oct 3, 2024

RobertWesner commented Oct 3, 2024 • edited Loading

RobertWesner commented Oct 5, 2024

simonhamp commented Oct 14, 2024

gwleuverink commented Sep 23, 2024 •

edited

Loading

gwleuverink Sep 24, 2024 •

edited

Loading

RobertWesner commented Oct 2, 2024 •

edited

Loading

RobertWesner commented Oct 3, 2024 •

edited

Loading