-
Notifications
You must be signed in to change notification settings - Fork 22
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] XMLTemplateAnalyzer: Support core:require #304
Conversation
Starting with UI5 1.69, a core:require attribute can be used in XML views/fragments to require modules/controls to be used within the XML context. SAP/openui5@2dab48e This change supports detecting those modules to be included in bundles.
lib/lbt/utils/JSTokenizer.js
Outdated
this.next(); | ||
}; | ||
|
||
// /** |
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.
Wouldn't it be possible to exclude the file from the JSDoc build of the tooling? At least this would simplify regular updates...
Well, the modification of the module definition remains necessary anyhow.
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.
Yes, that's right.
I mainly wanted to get npm test
working and discuss the re-use/integration of JSTokenizer beforehand.
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.
Done
log.error("Require attribute can't be parsed on Node: ", node.$ns.local); | ||
throw e; | ||
} | ||
if (requireContext) { |
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.
in the runtime code, we have added some validation for the object literal (keys must be simple identifiers, values must be string literals). At least for the requireJsName, a check for string literal might make sense...
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.
Good point 👍
Tests for those cases should also be added.
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.
Done
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.
Core change is fine with me, just some wording complaints in the test.
@@ -3,7 +3,7 @@ const XMLTemplateAnalyzer = require("../../../../lib/lbt/analyzer/XMLTemplateAna | |||
const ModuleInfo = require("../../../../lib/lbt/resources/ModuleInfo"); | |||
const sinon = require("sinon"); | |||
|
|||
test("integration: Analysis of an xml view", async (t) => { | |||
test("integration: Analysis of a xml view", async (t) => { |
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.
an?
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.
AFAIK, "an" has to be used whenever the next following word starts with a vocal when spoken, not when written. And "XML" is spoken "EX M L". Much like "SAP" is spoken "ESS AY PEE". That's why we also write "an sap.m.Button".
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.
Actually didn't know that, thanks 👍
@@ -33,7 +33,7 @@ test("integration: Analysis of an xml view", async (t) => { | |||
"Implicit dependency should be added since an XMLView is analyzed"); | |||
}); | |||
|
|||
test("integration: Analysis of an xml view with data binding in properties", async (t) => { | |||
test("integration: Analysis of a xml view with data binding in properties", async (t) => { |
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.
an?
@@ -58,7 +58,103 @@ test("integration: Analysis of an xml view with data binding in properties", asy | |||
"Implicit dependency should be added since an XMLView is analyzed"); | |||
}); | |||
|
|||
test("integration: Analysis of an xml fragment", async (t) => { | |||
test("integration: Analysis of a xml view with core:require", async (t) => { |
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.
an?
"Implicit dependency should be added since an XMLView is analyzed"); | ||
}); | ||
|
||
test("integration: Analysis of a xml view with core:require (parsing error)", async (t) => { |
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.
an?
"Implicit dependency should be added since an XMLView is analyzed"); | ||
}); | ||
|
||
test("integration: Analysis of a xml view with core:require (invalid module name)", async (t) => { |
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.
an?
"Implicit dependency should be added since an XMLView is analyzed"); | ||
}); | ||
|
||
test("integration: Analysis of a xml fragment", async (t) => { |
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.
an?
|
||
oTokenizer2.init("{='other foo'}"); | ||
t.true(oTokenizer2.getIndex() !== oTokenizer.getIndex(), "different instances"); | ||
}); |
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.
Nice that you test it in NodeJS again - makes sense.
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
Starting with UI5 1.69, a core:require attribute can be used in XML
views/fragments to require modules/controls to be used within the XML
context.
SAP/openui5@2dab48e
This change supports detecting those modules to be included in bundles.
Open Issues