Fix v2 build papercuts #26
Conversation
after first serve the package.json is patched and it will show the user's app name + version. But on first run it showed NativePHP@1.0.0
| "name": "NativePHP", | ||
| "version": "1.0.0", | ||
| "name": "nativephp", | ||
| "version": "v2", |
There was a problem hiding this comment.
To maintain the version numbering, should this be 2.0.0?
| "version": "v2", | |
| "version": "2.0.0", |
There was a problem hiding this comment.
these values are patched on an native:run & native:build. So this is basically only for the very first time native:install runs in a fresh project.
You're right it should be 2.0.1. But since v1 this was never updated and was always out of sync with the real version. Since it's only shown one time i figured this was fine and we can forget about it
There was a problem hiding this comment.
Scratch that, it's displayed after initial install too. My bad, It only updates in the cli if you also run npm update.
However I think it's fine still. I'm not keen on updating a single file on every new version
| $filesystem->dumpFile("{$buildPath}/storage/logs/_native.json", '{}'); | ||
| } | ||
|
|
||
| private function keepLivewireDispatcher() |
There was a problem hiding this comment.
Just a suggestion, but to avoid a method for every file, perhaps this could be named keepRequiredFiles(), similar to keepRequiredDirectories() above and could accept an array of files to keep (also opens up a potential for config to allow users to specify others)
There was a problem hiding this comment.
Thanks for the suggestion I agree with you there. That's what I did at first. My perspective changed a bit; this moving piece should not even exist in the first place. So I took it out into it's own method so I can rip it out more easily later on.
I hope to update the copying logic a so I can simply add this file using the glob patterns (exclude_list) instead. But I thought that change is a bit too risky to do as a patch right now.
What do you think?
There was a problem hiding this comment.
I think with good testing it's safe to do. But if this is a quick patch-release to resolve blocking issues now, I'm okay with it.
I definitely think this should be revisited and it sounds like you have a plan to do so. All good from me.
There was a problem hiding this comment.
Yes 100% I want to revisit this. This is a bit of a thorn in my eye, but should fix the issue reliably for people asap.
|
Thanks for giving it such a detailed look @PeteBishwhip 👍🏻 Merging now so I can continue with some other fixes. Will revisit the exclude file algo right after |
|
This PR was later superseded by the approach in #28 |
This PR fixes a several papercuts:
NativePHP@v1.0.0- updated tonativephp@v2Fixes #19