enhancement / address build command papercuts

[gwleuverink]

This PR adresses a couple of papercuts I've found in the build/publish commands.

Fixed

1. Composer install & cleanup env file were done before the app was copied to the build directory
   • When the build command finished, all dev dependencies were removed from the base path, breaking some things locally (and in my native:dev script)
   • When the build exits prematurely, the backup env file would not be restored

I've addressed this by porting the app copy logic to a php trait. This way the app is copied over before the electron-builder is executed. The copy step inside the electron-builder has been removed in favor for this approach. This way we don't have to modify any userland code.

2. Files that could contain sensitive information were not removed before build

During the copy step I account for files and directories that should be skipped. I've used an internal list here, which merges with the list of globs in the nativephp config file.

3. When building all mac architectures at once the mac-latest.yaml was overwritten by the last command

I've tried fixing that before in PR #145. Sadly this caused the last architecture not to compile with the correct php version

I've addressed this by removing the php building step as from the package.json scripts, and rather embed them in the build process by using a beforePack hook.

Other points for your consideration

1. When GITHUB_REPO, GITHUB_OWNER, GITHUB_PRIVATE are cleaned up, the publish command cannot upload to github.

They are all caught by the GITHUB_* wildcard. I assume this assume is true for the other update providers too. Maybe the default list of env keys to be cleaned should be more specific, like: GITHUB_TOKEN? (that's the only important one that needs to be cleared anyway)

I'm not sure if this still is an issue after these changes. Since we're not directly updating the projects' main env file anymore.

2. Logs do not rotate. File size can increase dramatically

Should we default to LOG_STACK=daily when building the app?

3. Should we also clean the node_modules directory when building?

This can become unwieldy quick! A fresh Laravel install already comes with a 30mb node_modules directory.
Not sure if we want to keep node_modules/.bin in this case.. An argument could be made to keep that around.

4. The x64 mac build is packaged with a x86 php binary

Not sure if this is by design. php.js was invoked with the --x86 flag, while electron builder was invoked with --mac --x64 in the build:mac-x86 script.

I kept this behaviour in, but not sure why it is like this

[gwleuverink]

I'm sure this needs more polish, but I'd appreciate a review since I'm changing around quite a bit of stuff @simonhamp

Don't want to make a wrong turn & have to backtrack. Appreciate it! 🙏🏻

[gwleuverink]

Tests didn't pass for php 8.1 because I used some some incompatible syntax

[simonhamp]

When GITHUB_REPO, GITHUB_OWNER, GITHUB_PRIVATE are cleaned up, the publish command cannot upload to github.

As long as the uploader is using the development .env and not the one in the build directory, then this shouldn't be a problem now, right?

Should we default to LOG_STACK=daily when building the app?

Yes. With a pretty short lifespan, so that logs don't hang around too long.

Should we also clean the node_modules directory when building?

I think in the majority case, whatever the developer was using it for should have been done before the build occurs, so I think there's a good case for just removing it all.

That said, it's not always so clear. I think we need to offload some of this to the developer to consider and provide appropriate config options to make it easy for them to define the behaviour they require.

I also think vendor/bin could be another candidate for being removed - highly unlikely that you'll want stray executables knocking around.

The x64 mac build is packaged with a x86 php binary

See my note in my review.

[simonhamp]

This is great. I haven't tested yet, just providing some feedback as requested

[gwleuverink]

Thanks for the review 👍

Sidenote. How do you feel about using the Laravel prompts spinner to give the build command some extra oomph?

[simonhamp]

Sounds fine but we need to check it falls back to something reasonable on Windows

[simonhamp]

I will try to review this properly in the next few days, but overall it feels great @gwleuverink, nice work!

@SRWieZ re build directory: I agree. Let's do it, let's bring builds out of vendor - in another PR

[gwleuverink]

@SRWieZ thanks for your feedback!

I definetly noticed some ide slowdowns and crashes. Let's pick that up in the next PR.

I'll go through your other points (php-bin & moving app name) next.

---

There seems to be one test that won't run in windows. But otherwise this PR is good to go

[gwleuverink]

Note: the boot test doesn't run also. But I suspect it's unrelated to this PR.

[gwleuverink]

The version in the Laravel repo wasn't compatible with the matrix we're running. Needed a bump to prevent composer errors in CI

[gwleuverink]

Okay. We're here.
All of this PR's tests are green.

Couple things to check - both unrelated to this PR.

• Plugin test & plugin build fail - I believe due to a upstream change
• BootingTest doesn't run in ubuntu

Otherwise I think this is good to go.

[gwleuverink]

Note: We need to double check if the change Pete made in PR #162 isn't overwritten by merging this