-
-
Notifications
You must be signed in to change notification settings - Fork 605
XCOMMONS-3470: Easier importmap declaration for webjars #4737
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
Conversation
...rtmap/src/main/java/org/xwiki/javascript/importmap/internal/JavascriptImportmapResolver.java
Show resolved
Hide resolved
xwiki-platform-core/xwiki-platform-livedata/xwiki-platform-livedata-webjar/pom.xml
Outdated
Show resolved
Hide resolved
| <xwiki.minification.skip>true</xwiki.minification.skip> | ||
| <xwiki.javascript.modules.importmap> | ||
| { | ||
| "xwiki-livedata": "org.xwiki.platform:xwiki-platform-livedata-webjar/main.es.js", |
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.
I'm wondering if it's not a bit too simple, I mean you might need more information that just the extension id and path (the webjars URL API can take custom parameters, for example).
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.
More than what "xwiki-livedata": "org.xwiki.platform:xwiki-platform-livedata-webjar/main.es.js?myparam=42" could do?
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.
@tmortagne what I'm concerned with for now is that I have no idea how to load this parameter programmatically in the accessProperty method (here https://github.com/xwiki/xwiki-platform/pull/4737/files#diff-56da785aaad1d5f483a61edaa411ef6836fbddfbb368b32d3349a0abb80e923cR167).
See also this message where I try to describe the problem https://matrix.to/#/!zdPfnFC0pUfwN5kZ4LKP2dXZ2jC266yFWKNeqgmJd4E/$Juhc3kpzhi9XiQbQ2iSckpO6wM-qfq4kD_2w31kVUyA?via=matrix.xwiki.com&via=matrix.org&via=uni-wuppertal.de
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.
Right, I forgot to mention that only the Maven properties which name starts with "xwiki.extension." are extracted from the pom.xml to be exposed as Extension property. So you will need something which looks more like xwiki.extension.javascript.modules.importmap I guess.
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.
More than what "xwiki-livedata": "org.xwiki.platform:xwiki-platform-livedata-webjar/main.es.js?myparam=42" could do?
Since you use JSON, it might be less error-prone to fully use JSON syntax to define all this.
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.
I changed the format to also support namespace and parameters.
moduleA is an example of the compact and straighforward declaration of a simple webjar with a path.
But as you can see with moduleB, the string can be replaced with an object with 4 keys:
webjarIdandpathare mandatorynamespaceandparamsare optionals
{
"moduleA": "org.xwiki.js/thepath.js",
"moduleB": {
"webjarId": "org.xwiki.js",
"path": "path.js",
"namespace": "s1",
"params": {
"key": "value"
}
}
}
mflorea
left a comment
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.
I'm looking forward to using this feature.
.../src/main/java/org/xwiki/javascript/importmap/internal/JavascriptImportmapEventListener.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/xwiki/javascript/importmap/internal/JavascriptImportmapEventListener.java
Outdated
Show resolved
Hide resolved
...rtmap/src/main/java/org/xwiki/javascript/importmap/internal/JavascriptImportmapResolver.java
Show resolved
Hide resolved
- Adapt to resolve per-wiki - change the configuration key to be compatible with the extensions API
bd27042 to
b9d41e8
Compare
| .configure(SORT_PROPERTIES_ALPHABETICALLY, true) | ||
| .build(); | ||
|
|
||
| private static final JavascriptImportmapParser JAVASCRIPT_IMPORTMAP_PARSER = new JavascriptImportmapParser(); |
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.
The parser cannot be a component as I'd also like it to be usable in the maven enforcers who do not have access to components injection.
| import java.util.Map; | ||
| import java.util.Objects; | ||
|
|
||
| import javax.inject.Named; |
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.
@tmortagne if jakarta is used instead, the enforcer is not found.
Mentioning it in case this is not a know issue.
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.
I guess it's more something to report to the enforcer plugin (this is not an XWiki component/feature).
| var isSelf = | ||
| areDependenciesEquals(model.getGroupId(), model.getArtifactId(), groupIdWebjar, artifactIdWebjar); | ||
| if (isSelf) { | ||
| continue; |
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.
One last issue: I'm not sure how to find out if the path exists when the map defines the current artifact itself since the target directory is not populated when the check is performed.
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.
Solved by changing the enforcer's phase, see https://github.com/xwiki/xwiki-platform/pull/4737/files#diff-2f81214bd08060b9f2f1bdd7cb0dda0e7109ccadbf852cda6ddb864abc29c8e3R214
Jira URL
Changes
Description
Clarifications
Screenshots & Video
Executed Tests
Expected merging strategy
*