-
Notifications
You must be signed in to change notification settings - Fork 295
Consolidate build and publish scripts #25
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
Conversation
tar -xzf wordpress-6.0.1.tar.gz | ||
rm wordpress-6.0.1.tar.gz | ||
# Download latest version of WordPress | ||
wp_tarfile=wordpress-latest.tar.gz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel pretty confident about defaulting to the latest minor release, e.g. 6.0.2
. Auto-upgrading to the next major release like 6.1.0
could break the build, though, if anything essential changes. I'd rather keep doing this manually every few months until there are more checks and balances in place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense - that would be more stable and reliable than always pulling the newest version automatically. OK, updated.
Great PR @eliot-akira, the code reads well! I'll also test it soon. Debugging #1 made my local setup quite unwieldy, but I will give this PR some testing soon. |
Thank you for the review! I'll make another commit so the WordPress download is pinned to a specific version. In that case, what do you think about "caching" the tar file (by not removing it every time)? wp_tarfile=wordpress-6.0.2.tar.gz
if [ ! -f $wp_tarfile ]; then
wget https://wordpress.org/$wp_tarfile
fi
tar -xzf $wp_tarfile The file could be added to As an aside - I've continued trying to solve #24 with a "hand shake" between app.mjs and service worker (event message and response, then set iframe src) - but ran into difficulty with some fetch events from the iframe that don't have client ID for some reason, that cannot be distinguished from fetch events from the host. For this I'm testing on Chromium, and have observed #1 on occasion, "Aw snap". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great contribution as always @eliot-akira! 👍 I'm not merging yet as there is an ongoing discussion, but I'm approving already.
Thanks! This last change (mainly commit 6733446) turned out to be bigger than I expected. It'd be great if you could test drive the scripts to check that they all do everything as expected, same as before. I reorganized the
|
It all works in my testing. There were a few merge conflicts with #28 so I went ahead and rebased this PR. Splendid work @eliot-akira, the scripts structure feels much cleaner now. Thank you so much! |
Resolves #15. The NPM scripts were getting long and unwieldy to edit inside
package.json
, so I moved them into separate files. I made a couple of other small improvements along the way.