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

[FEATURE] Bundle require section with async flag for specVersion: 4.0 #1042

Merged
merged 9 commits into from
Jul 5, 2024

Conversation

d3xter666
Copy link
Contributor

@d3xter666 d3xter666 commented Jul 2, 2024

JIRA: CPOUI5FOUNDATION-835

This change completes #1033 with check for specVersion: 4.0 and enforcing default async flag for require section in bundleDefinitions

@d3xter666 d3xter666 requested a review from a team July 2, 2024 06:28
@coveralls
Copy link

Coverage Status

coverage: 94.678% (+0.09%) from 94.588%
when pulling 5959c01 on bundleDefs-by-spec-version
into f119ccd on main.

@coveralls
Copy link

Coverage Status

coverage: 94.678% (+0.09%) from 94.588%
when pulling dcb3155 on bundleDefs-by-spec-version
into f119ccd on main.

@d3xter666
Copy link
Contributor Author

d3xter666 commented Jul 2, 2024

The build fails as for some reason Azure tries to install Node 20.15.0, which has some isseues with Buffers.
With that particular version the issue is also reproducible locally.

Edit: Fixed with #1043

@coveralls
Copy link

Coverage Status

coverage: 94.677% (+0.009%) from 94.668%
when pulling bb33e24 on bundleDefs-by-spec-version
into 934956a on main.

@d3xter666 d3xter666 requested a review from RandomByte July 2, 2024 09:54
@coveralls
Copy link

Coverage Status

coverage: 94.677% (+0.009%) from 94.668%
when pulling 50a1979 on bundleDefs-by-spec-version
into 934956a on main.

@coveralls
Copy link

Coverage Status

coverage: 94.677% (+0.009%) from 94.668%
when pulling 7916be7 on bundleDefs-by-spec-version
into 934956a on main.

@flovogt flovogt merged commit dfa67fe into main Jul 5, 2024
17 checks passed
@flovogt flovogt deleted the bundleDefs-by-spec-version branch July 5, 2024 11:39
@@ -135,6 +135,7 @@ const log = getLogger("builder:processors:bundlers:moduleBundler");
* @param {string} [parameters.options.targetUi5CoreVersion] Optional semver compliant sap.ui.core project version, e.g '2.0.0'.
This allows the bundler to make assumptions on available runtime APIs.
Omit if the ultimate UI5 version at runtime is unknown or can't be determined.
* @param {@ui5/project/build/helpers/TaskUtil|object} [parameters.taskUtil] TaskUtil
Copy link
Member

Choose a reason for hiding this comment

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

obsolete parameter? or was it already in use but undocumented?

Copy link
Member

Choose a reason for hiding this comment

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

// If specVersion cannot be determined, the latest spec is taken into account.
// This is a breaking change in specVersion: 4.0
if (section.mode === "require" && (!taskUtil || taskUtil.getProject().getSpecVersion().gte("4.0"))) {
defaultValues.async = true;
Copy link
Member

Choose a reason for hiding this comment

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

Is there a test that verifies this behavior?
The bundle tooling already defaults to "async=true", so I doubt that this line has any effect. My expectation would be that the flag has to be actively set to false in the else case, as this is the value that differs from the new default behavior.

I tested this with a self-contained build and even with specVersion 3.0 I get an async require section, which is not expected.

Copy link
Member

Choose a reason for hiding this comment

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

We should also document the "async" option in the respective JSDoc: https://github.com/SAP/ui5-builder/blob/main/lib/processors/bundlers/moduleBundler.js#L13

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

Successfully merging this pull request may close these issues.

5 participants