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.

Sign up for GitHub

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] Export build processor manifestCreator #648

Merged
merged 4 commits into from
Oct 14, 2021
Merged

[INTERNAL] Export build processor manifestCreator #648

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
aecb9ad
[INTERNAL] Export build processor manifestCreator
larskissel Oct 4, 2021
7b0b065
[INTERNAL] Add manifestCreator option to make minUI5Version optional
larskissel Oct 11, 2021
ff8cd13
[INTERNAL] Improve new manifestCreator option as suggested in code re…
larskissel Oct 12, 2021
bc97b3c
[INTERNAL] Adapt test as suggested
larskissel Oct 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ module.exports = {
* @type {import('./lib/processors/libraryLessGenerator')}
*/
libraryLessGenerator: "./lib/processors/libraryLessGenerator",
/**
* @type {import('./lib/processors/manifestCreator')}
*/
manifestCreator: "./lib/processors/manifestCreator",
/**
* @type {import('./lib/processors/resourceCopier')}
*/
Expand Down
16 changes: 11 additions & 5 deletions lib/processors/manifestCreator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ class LibraryBundle {
/*
* Creates the library manifest.json file for a UILibrary.
*/
async function createManifest(libraryResource, libBundle, descriptorVersion, _include3rdParty) {
async function createManifest(libraryResource, libBundle, descriptorVersion, _include3rdParty, minUI5VersionRequired) {
// create a Library wrapper around the .library XML
const library = await Library.from(libraryResource);

Expand Down Expand Up @@ -604,7 +604,7 @@ async function createManifest(libraryResource, libBundle, descriptorVersion, _in
}

function normalizeVersion(version) {
if ( version == null ) {
if ( version == null || !minUI5VersionRequired && !version ) {
return version;
}
const v = new Version(version);
Expand Down Expand Up @@ -634,6 +634,10 @@ async function createManifest(libraryResource, libBundle, descriptorVersion, _in
}
}

if ( !minUI5VersionRequired ) {
Copy link
Member

Choose a reason for hiding this comment

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

I think the getVersion function is used for more than the minUI5Version. Why not move this if into the getUI5Version function instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

See discussion below #648 (comment)

return "";
}

throw new Error(
`Couldn't find version for library '${dependency.getLibraryName()}', project dependency missing?`);
}
Expand All @@ -652,9 +656,10 @@ async function createManifest(libraryResource, libBundle, descriptorVersion, _in
module.exports = function({libraryResource, resources, options}) {
// merge options with defaults
options = Object.assign({
descriptorVersion: APP_DESCRIPTOR_V22, // TODO change this to type string instead of a semver object
descriptorVersion: APP_DESCRIPTOR_V22, // TODO 3.0: change this to type string instead of a semver object
include3rdParty: true,
prettyPrint: true
prettyPrint: true,
minUI5VersionRequired: true
}, options);

const resourcePathPrefix = libraryResource.getPath().slice(0, -".library".length);
Expand All @@ -667,7 +672,8 @@ module.exports = function({libraryResource, resources, options}) {
return Promise.resolve(null); // a fulfillment of null indicates that no manifest has been created
}

return createManifest(libraryResource, libBundle, options.descriptorVersion, options.include3rdParty)
return createManifest(libraryResource, libBundle, options.descriptorVersion, options.include3rdParty,
options.minUI5VersionRequired)
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer a switch to simply omit the minUI5Version instead of trying to resolve it. Maybe omitMinUI5Version?

In the ui5-server use case, there is no benefit from having the minUI5Version available in some cases. So I think it should never be available in order to keep things simpler.

Copy link
Member Author

Choose a reason for hiding this comment

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

As discussed, let's call it omitMinVersions because not only the minUI5Version but also the minVersion of dependencies could lead to issues. The added versionInfoGenerator test proves that missing (empty string) minVersion/minUI5Version info doesn't affect the result of a sap-ui-version.json

.then((manifest) => {
return new EvoResource({
path: resourcePathPrefix + "manifest.json",
Expand Down
68 changes: 68 additions & 0 deletions test/lib/processors/manifestCreator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,74 @@ test.serial("default manifest creation no dependency version", async (t) => {
"Couldn't find version for library 'my.lib', project dependency missing?", "error message correct");
});

test.serial("manifest creation no minUI5Version required", async (t) => {
const {manifestCreator, errorLogStub} = t.context;

const expectedManifestContent = JSON.stringify({
"_version": "1.21.0",
"sap.app": {
"id": "library.e",
"type": "library",
"embeds": [],
"applicationVersion": {},
"title": "Library E",
"description": "Library E",
"resources": "resources.json",
"offline": true
},
"sap.ui": {
"technology": "UI5",
"supportedThemes": []
},
"sap.ui5": {
"dependencies": {
"minUI5Version": "",
"libs": {
"my.lib": {
"minVersion": ""
}
}
},
"library": {
"i18n": false
}
}
}, null, 2);

const libraryResource = {
getPath: () => {
return "/resources/sap/ui/mine/.library";
},
getString: async () => {
return `<?xml version="1.0" encoding="UTF-8" ?>
<library xmlns="http://www.sap.com/sap.ui.library.xsd" >
<name>library.e</name>
<vendor>SAP SE</vendor>
<documentation>Library E</documentation>
<dependencies>
<dependency>
<libraryName>my.lib</libraryName>
</dependency>
</dependencies>
</library>`;
},
_project: {
dependencies: []
}
};

const result = await manifestCreator({
libraryResource,
resources: [],
options: {
minUI5VersionRequired: false
}
});

t.is(await result.getString(), expectedManifestContent, "Correct result returned");
t.is(errorLogStub.callCount, 0);
});

test.serial("default manifest creation with special characters", async (t) => {
const {manifestCreator, errorLogStub} = t.context;
const prefix = "/resources/sap/ui/mine/";
Expand Down