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

[FIX] Allow usage of after/before task assignment for all standard tasks #628

Merged
merged 10 commits into from
Sep 27, 2023

Conversation

flovogt
Copy link
Member

@flovogt flovogt commented Jul 6, 2023

This change allows custom tasks to subscribe to any standard task, even disabled ones, ensuring custom tasks' execution and the correct order.

Background information:
When transitioning a project from V2 to V3, we often need to adjust the before/after task assignments for custom tasks due to the removal of the uglify task. The new minify task now runs earlier than the previous uglify task.
Developers typically used uglify as the last enabled standard task to subscribe to.

You can find updated documentation here: SAP/ui5-tooling#874

JIRA: CPOUI5FOUNDATION-724

@flovogt flovogt force-pushed the allow-task-assignment-generateBundle branch 2 times, most recently from 12fa0d4 to 4ecef82 Compare September 4, 2023 11:20
@matz3 matz3 added the bug Something isn't working label Sep 4, 2023
When migrating an project from V2 to V3 the after/before task
assignments of custom tasks often has to be adjusted because the task
`uglify` got removed and new `minify` task is executed earlier as the
`uglify` task.

In the migration guide we have written that we can not make any
assumptions about the task which should be used as the new "last" task
executed by the standard UI5 Tooling build tasks.
See https://sap.github.io/ui5-tooling/stable/updates/migrate-v3/#removal-of-standard-tasks-and-processors.

In the above mentioned documentation we have a list with all tasks and
the execution order.
Currently, it works to assign a custom task after/before a task which
is not executed at all.
So you can assign a custom task to `afterTask: generateVersionInfo`
although the task `generateVersionInfo` is not executed.

This works for every standard UI5 Tooling build task except for
`generateBundle`.
This PR aligns the behaviour also for the `generateBundle` task.

JIRA: CPOUI5FOUNDATION-724
@matz3 matz3 force-pushed the allow-task-assignment-generateBundle branch from 4ecef82 to 4962e9e Compare September 5, 2023 11:47
@matz3 matz3 marked this pull request as ready for review September 5, 2023 11:53
@matz3 matz3 requested a review from d3xter666 September 5, 2023 11:53
Copy link
Contributor

@d3xter666 d3xter666 left a comment

Choose a reason for hiding this comment

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

I got from the description that we currently face issues only with generateBundle, but I also can see potential issues of the same kind in the following places:

  • lib/build/definitions/library.js
    • generateComponentPreload task
    • generateBundle task (already addressed in the comments)
    • generateThemeDesignerResources task
  • lib/build/definitions/themeLibrary.js
    • generateThemeDesignerResources task

I'm unsure whether we need to address those now, but they seem the same.

lib/build/definitions/application.js Show resolved Hide resolved
matz3
matz3 previously requested changes Sep 25, 2023
Copy link
Member

@matz3 matz3 left a comment

Choose a reason for hiding this comment

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

I missed some detail when recommending to just use null as taskFunction.
The task logger used in runTasks must only receive the list of task which are actually executed and logged. Otherwise it will wait forever and the build never succeeds (you should be able to verify that e.g. with openui5-sample-app).

So the allTasks list must not include tasks which should not be executed. So the initial solution should be fine (using composeTaskList) as long as the logic is added after the include/excludes are handled because such a no-op task should never be enabled.

@matz3 matz3 changed the title [FIX] Allow usage of after/before task assignment of generateBundle [FIX] Allow usage of after/before task assignment for all standard tasks Sep 26, 2023
@matz3 matz3 dismissed their stale review September 26, 2023 12:46

Comments have been addressed

@d3xter666 d3xter666 merged commit 1a272d2 into main Sep 27, 2023
17 checks passed
@d3xter666 d3xter666 deleted the allow-task-assignment-generateBundle branch September 27, 2023 12:37
d3xter666 added a commit to SAP/ui5-tooling that referenced this pull request Oct 2, 2023
JIRA: CPOUI5FOUNDATION-724

This PR aims to improve the documentation by providing correct
information based on a related change for custom tasks subscription &
execution order behaviour: SAP/ui5-project#628

---------

Co-authored-by: Günter Klatt <57760635+KlattG@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants