-
Notifications
You must be signed in to change notification settings - Fork 23
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
[INTERNAL] Async bundling for require section #1048
Conversation
Please use [INTERNAL] as this fixes an issue of a feature that hasn't been released, so it is not relevant to be mentioned in the changelog. |
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.
Works as expected 👍🏻
Please also keep @RandomByte's comment in mind: #1042 (comment) |
@KlattG , would you take a look at this one, please? |
ae0fb09
to
0a99344
Compare
The build fails as we need to publish specVersion: 4.0 in order tests to run successfully |
Tests already fail in main as we're about to do the release. cc @flovogt Independent from that there are also two eslint errors introduced with this PR. |
Hi @KlattG, I have updated the texts according to the comments above. |
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.
Just one small suggestion, otherwise fine.
@@ -75,6 +75,10 @@ const log = getLogger("builder:processors:bundlers:moduleBundler"); | |||
* @property {boolean} [declareRawModules=false] Whether raw modules should be declared after jQuery.sap.global | |||
* became available. With the usage of the ui5loader, this flag should be set to 'false' | |||
* @property {boolean} [sort=true] Whether the modules should be sorted by their dependencies | |||
* @property {boolean} [async=true] Whether the `require` section of the module should be loaded asynchronously. | |||
* When set to true, the modules are loaded using `sap.ui.require` instead of `sap.ui.requireSync`. |
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.
Should we mention that each section creates one sap.ui.require call (or alternatively, that the modules are required in parallel)?
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.
LGTM
Co-authored-by: Günter Klatt <57760635+KlattG@users.noreply.github.com>
Co-authored-by: Günter Klatt <57760635+KlattG@users.noreply.github.com>
Co-authored-by: Günter Klatt <57760635+KlattG@users.noreply.github.com>
Co-authored-by: Günter Klatt <57760635+KlattG@users.noreply.github.com>
Co-authored-by: Günter Klatt <57760635+KlattG@users.noreply.github.com>
Co-authored-by: Günter Klatt <57760635+KlattG@users.noreply.github.com>
e0f8451
to
a06c99a
Compare
Require's section
async
flag has different default values based on the specVersion. However, async require is now build by default.We need to explicitly default
async=false
for specVersions lower than 4.0