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

feat: support sap.ui.require for @sapUiRequire annotated modules #131

Merged
merged 1 commit into from
Jul 22, 2024

Conversation

petermuessig
Copy link
Member

@petermuessig petermuessig commented Jul 13, 2024

QUnit testsuites using QUnit.config.autostart = false and QUnit.start()
loaded via <script> tags must be loaded using sap.ui.require. With this
change modules can be marked using sap.ui.require by using the annotation
/* @sapUiRequire */ in the program code.

Copy link
Collaborator

@codeworrior codeworrior left a comment

Choose a reason for hiding this comment

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

As explained in our chat, this is not valid as a general replacement. The module loading does not wait for sap.ui.require to complete before consumers (modules that depend on the current module) continue to execute. If there are side effects other than the return value (e.g. class creation, data type registration, module loading (probing)..., usages of native APIs that can be noticed by the outside), then the timing is most likely broken with this change.

See also "Using sap.ui.require instead of sap.ui.define on the top level" in
Troubleshooting for Loading Modules
.

@petermuessig petermuessig changed the title feat: modules with no exports use sap.ui.require feat: support sap.ui.require for QUnit testsuites or annotated modules Jul 14, 2024
@petermuessig
Copy link
Member Author

@codeworrior - thx for the explanation - I updated the change now to either autodetect the QUnit testsuite or to use an annotation to mark a module using sap.ui.require.

@petermuessig petermuessig requested a review from codeworrior July 14, 2024 10:14
@petermuessig petermuessig force-pushed the feat/SapUiRequire branch 2 times, most recently from ac0078b to 51daf03 Compare July 15, 2024 07:55
@petermuessig petermuessig force-pushed the feat/SapUiRequire branch 2 times, most recently from b3586f8 to 45727bf Compare July 15, 2024 10:48
@petermuessig petermuessig requested a review from codeworrior July 15, 2024 11:52
@petermuessig petermuessig changed the title feat: support sap.ui.require for QUnit testsuites or annotated modules feat: support sap.ui.require for @sapUiRequire annotated modules Jul 15, 2024
Copy link
Contributor

@akudev akudev left a comment

Choose a reason for hiding this comment

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

Good overall, I just have sort of consistency concerns: for class transformation, the name(space) can be given as either JSDoc tag in a comment or as decorator.

For the old controller extensions, the instruction could be given as either JSDoc-like tag in any kind of comment or as decorator. (cf. https://github.com/ui5-community/babel-plugin-transform-modules-ui5/blob/main/packages/plugin/src/classes/helpers/classes.js#L163-168).

However, here it must be a JSDoc-like tag in a block comment. No decorator, no line comment.

The old controller extension approach was always private and will be removed again, so it is only the class decoration to which consistency would be nice. And decorators might not even make sense here, as there may be nothing to decorate. But still, having to know it must be a block comment and not a line comment seems a bit less user-friendly than possible to me.

On the other hand, being able to add the comment anywhere in the code sounds very flexible, but I wonder whether this flexibility is actually needed. Would top-level or beginning-of-file be enough and easier to locate for users? A @sapUiRequire nested deeply in line 1500 might be missed by the developer wondering why the file does not work and is transformed in a different way.

This and the inline comment are just thoughts.

README.md Show resolved Hide resolved
@petermuessig
Copy link
Member Author

petermuessig commented Jul 15, 2024

@akudev - IMO, we can restrict it to just the first comment in the file. But what if there are also ESLint comments or others? Maybe ignoring nested ones is OK, but top-level comments should be at least ok. Maybe saying top-level comments in the head of the file?

@akudev
Copy link
Contributor

akudev commented Jul 15, 2024

@petermuessig Maybe it does not need to be the very first line but "before any code" or just "on top-level".

It also does not need to be an actual hard restriction: just using it close to the beginning in the samples and replacing the strange-sounding docu "somewhere in the program code" might also be an improvement already which makes people use it more consistently.

@petermuessig
Copy link
Member Author

Ok, I changed the comment to state that the comment should be added at the top of the file.

@petermuessig petermuessig requested a review from akudev July 19, 2024 20:29
QUnit testsuites using `QUnit.config.autostart = false` and `QUnit.start()`
loaded via <script> tags must be loaded using `sap.ui.require`. With this
change modules can be marked using `sap.ui.require` by using the annotation
`/* @sapUiRequire */` in the program code.
@petermuessig petermuessig merged commit 23c4ac6 into main Jul 22, 2024
6 checks passed
@petermuessig petermuessig deleted the feat/SapUiRequire branch July 22, 2024 09:04
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.

3 participants