fix(Makefile): add support for apps requiring composer with --no-scri… #94
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new build category COMPOSER_NO_SCRIPTS_WITH_NPM_APPS for external apps that require both composer installation with --no-scripts flag and npm build steps. This addresses a gap where apps like forms, password_policy, and twofactor_totp have composer script issues but also need JavaScript builds.
Key Changes:
- Added
COMPOSER_NO_SCRIPTS_WITH_NPM_APPScategory for apps requiring both composer (with --no-scripts) and npm build - Updated validation scripts to recognize and validate the new category
- Moved three apps (
forms,password_policy,twofactor_totp) to the new category
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Makefile | Added new COMPOSER_NO_SCRIPTS_WITH_NPM_APPS variable and build targets, moved three apps to this category |
| scripts/validate_external_apps.sh | Updated validation logic to detect and recommend the new category for apps with composer script issues that also need npm builds |
| scripts/validate_app_list_uniqueness.sh | Extended uniqueness validation to include the new category in all relevant checks |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…pts and npm build Updated validation scripts and Makefile to include a new category for apps that need composer with --no-scripts and npm build. This change ensures proper categorization and handling of these apps during the build process. Should be made in 53c525a Signed-off-by: Misha M.-Kupriyanov <[email protected]>
printminion-co
force-pushed
the
mk/dev/fix_forms_js_build
branch
from
c6d725f to
2d39abc
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Intentionally left empty: currently, no apps require composer with --no-scripts only. | ||
| COMPOSER_NO_SCRIPTS_APPS = |
There was a problem hiding this comment.
The variable COMPOSER_NO_SCRIPTS_APPS is now empty and appears to have no apps assigned to it. If this category is truly no longer needed, consider removing the variable definition and all related build targets/rules to avoid maintaining unused code. Alternatively, if this is temporary, the comment should clarify the expected future state.
| echo " (Use this for apps with @composer bin commands but bamarni plugin only in require-dev)" | ||
| echo "" | ||
| echo " For COMPOSER_NO_SCRIPTS_WITH_NPM_APPS (apps with composer scripts issues + npm build):" | ||
| echo " Add to line 67: COMPOSER_NO_SCRIPTS_WITH_NPM_APPS = \\" |
There was a problem hiding this comment.
The line number reference '67' may become incorrect if the Makefile is modified. Consider using a more maintainable approach, such as referencing the variable name pattern or removing specific line numbers from the instruction.
| echo " (Use this for apps with @composer bin/bamarni issues that also need npm build)" | ||
| echo "" | ||
| echo " For NOTHING_TO_BUILD_APPS (no build steps needed):" | ||
| echo " Add to line 73: NOTHING_TO_BUILD_APPS = \\" |
There was a problem hiding this comment.
The line number reference '73' may become incorrect if the Makefile is modified. Consider using a more maintainable approach, such as referencing the variable name pattern or removing specific line numbers from the instruction.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…pts and npm build
Updated validation scripts and Makefile to include a new category for apps that need composer with --no-scripts and npm build. This change ensures proper categorization and handling of these apps during the build process.
JS of forms, password_policy, twofactor_totp is build now
Should be made in 53c525a