Skip to content
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

Build the app before running the prep-release script #1477

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yuliyan
Copy link
Member

@yuliyan yuliyan commented May 22, 2024

Changes proposed in this Pull Request:

The update-translations command of the prep-release script will scan the directory for translatable strings, including the build directory, which can either be outdated or not existing at all, resulting in excluding strings from the generated POT file.

This PR updates the npm run prepare-release script to add npm run build prior to executing the prep-release.js script to ensure the build directory is up to date when scanning for translatable strings.

  • This PR is a very minor change/addition and does not require testing instructions (if checked you can ignore/remove the next section).

How to test the changes in this Pull Request:

  1. Code review.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?

FOR PR REVIEWER ONLY:

  • I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

Copy link

Size Change: 0 B

Total Size: 197 kB

ℹ️ View Unchanged
Filename Size
./build/53.js 1.05 kB
./build/index.css 883 B
./build/index.js 122 kB
./build/marketing.js 58 kB
./build/payment-gateway-suggestions.css 1.25 kB
./build/payment-gateway-suggestions.js 6.46 kB
./build/plugins.js 3.92 kB
./build/style-index.css 2.15 kB
./build/style-marketing.css 805 B

compressed-size-action

@d-alleyne d-alleyne mentioned this pull request May 22, 2024
5 tasks
Copy link
Contributor

@d-alleyne d-alleyne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that we have to remove the node modules directory, so that it isn't packaged up in the release afterwards?

@yuliyan yuliyan changed the title Build the app before running the prep-realase script Build the app before running the prep-release script May 27, 2024
@yuliyan
Copy link
Member Author

yuliyan commented May 27, 2024

Does this mean that we have to remove the node modules directory, so that it isn't packaged up in the release afterwards?

The node_modules directory is removed when building the app in the create-release script.

@d-alleyne
Copy link
Contributor

d-alleyne commented May 27, 2024

Does this mean that we have to remove the node modules directory, so that it isn't packaged up in the release afterwards?

The node_modules directory is removed when building the app in the create-release script.

I simulated a pre-release by bumping the version and updating the changelog, but exited the process before creating the pull request.

diff --git a/.sshyncignore b/.sshyncignore  
new file mode 100644  
index 0000000..353e57b  
--- /dev/null  
+++ b/.sshyncignore  
@@ -0,0 +1,7 @@  
+.git  
+node_modules/  
+tests/  
+vendor/  
+.cache/  
+.idea  
+.config  

It seems that the Git commands in these scripts are adding files that were not initially tracked, or were even marked to be ignored in the .gitignore file.

Other than that, LGTM!

@yuliyan
Copy link
Member Author

yuliyan commented May 28, 2024

@markbiek, since you've worked on the script before, could you confirm if it's ok to build the app before running the prep-release.js script?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants