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

[FEATURE] buildThemes: Add filtering for available themes #419

Merged
merged 8 commits into from
Mar 6, 2020
Merged
Changes from 1 commit
Commits
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
Prev Previous commit
Next Next commit
Adoptions
  • Loading branch information
matz3 committed Mar 4, 2020

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
commit dc93bf8f95c81cd66eeb4679e907515278fcb8cd
15 changes: 12 additions & 3 deletions lib/builder/BuildContext.js
Original file line number Diff line number Diff line change
@@ -5,8 +5,13 @@
* @memberof module:@ui5/builder.builder
*/
class BuildContext {
constructor() {
constructor({rootProject}) {
this.projectBuildContexts = [];
this.rootProject = rootProject;
}

getRootProject() {
return this.rootProject;
}

createProjectContext({project, resources}) {
@@ -36,14 +41,18 @@ class BuildContext {
*/
class ProjectBuildContext {
constructor({buildContext, project, resources}) {
// this.buildContext = buildContext;
// this.project = project;
this._buildContext = buildContext;
this._project = project;
// this.resources = resources;
this.queues = {
cleanup: []
};
}

isRootProject() {
return this._project === this._buildContext.getRootProject();
}

registerCleanupTask(callback) {
this.queues.cleanup.push(callback);
}
8 changes: 3 additions & 5 deletions lib/builder/builder.js
Original file line number Diff line number Diff line change
@@ -235,7 +235,7 @@ module.exports = {
virBasePath: "/"
});

const buildContext = new BuildContext();
const buildContext = new BuildContext({rootProject: tree});
const cleanupSigHooks = registerCleanupSigHooks(buildContext);

const projects = {}; // Unique project index to prevent building the same project multiple times
@@ -276,7 +276,6 @@ module.exports = {
const buildLogger = log.createTaskLogger("🛠 ", projectCount(tree));

function buildProject(project) {
const isRootProject = project === tree;
let depPromise;
let projectTasks = selectedTasks;

@@ -330,7 +329,7 @@ module.exports = {
});

const projectContext = buildContext.createProjectContext({
// project, // TODO 2.0: Add project facade object/instance here
project, // TODO 2.0: Add project facade object/instance here
resources: {
workspace,
dependencies: resourceCollections.dependencies
@@ -348,7 +347,6 @@ module.exports = {
},
tasks: projectTasks,
project,
isRootProject,
parentLogger: log,
buildContext: projectContext
}).then(() => {
@@ -357,7 +355,7 @@ module.exports = {

return workspace.byGlob("/**/*.*").then((resources) => {
return Promise.all(resources.map((resource) => {
if (isRootProject && project.type === "application" && project.metadata.namespace) {
if (projectContext.isRootProject() && project.type === "application" && project.metadata.namespace) {
// Root-application projects only: Remove namespace prefix if given
resource.setPath(resource.getPath().replace(
new RegExp(`^/resources/${project.metadata.namespace}`), ""));
6 changes: 5 additions & 1 deletion lib/tasks/buildThemes.js
Original file line number Diff line number Diff line change
@@ -16,6 +16,7 @@ const log = require("@ui5/logger").getLogger("builder:tasks:buildThemes");
* @param {string} parameters.options.projectName Project name
* @param {string} parameters.options.inputPattern Search pattern for *.less files to be built
* @param {string} [parameters.options.librariesPattern] Search pattern for .library files
* @param {string} [parameters.options.themesPattern] Search pattern for sap.ui.core theme folders
* @param {boolean} [parameters.options.compress=true]
* @param {boolean} [parameters.options.cssVariables=false]
Copy link
Member

Choose a reason for hiding this comment

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

New optional parameter themesPattern also needs to be documented here

* @returns {Promise<undefined>} Promise resolving with <code>undefined</code> once data has been written
@@ -90,7 +91,10 @@ module.exports = async function({workspace, dependencies, options}) {
log.verbose(`Skipping ${resourcePath}: Library is not available`);
}
if (!themeAvailable) {
log.verbose(`Skipping ${resourcePath}: sap.ui.core theme '${themeName}' is not available`);
log.verbose(`Skipping ${resourcePath}: sap.ui.core theme '${themeName}' is not available. ` +
"If you experience missing themes, check whether you have added the corresponding theme " +
"library to your projects dependencies and make sure that your custom themes contain " +
"resources for the sap.ui.core namespace.");
}
}

6 changes: 3 additions & 3 deletions lib/types/library/LibraryBuilder.js
Original file line number Diff line number Diff line change
@@ -19,7 +19,7 @@ const tasks = { // can't require index.js due to circular dependency
};

class LibraryBuilder extends AbstractBuilder {
addStandardTasks({resourceCollections, project, isRootProject, log, buildContext}) {
addStandardTasks({resourceCollections, project, log, buildContext}) {
if (!project.metadata.namespace) {
log.info("Skipping some tasks due to missing library namespace information. Your project " +
"might be missing a manifest.json or .library file. " +
@@ -181,8 +181,8 @@ class LibraryBuilder extends AbstractBuilder {
dependencies: resourceCollections.dependencies,
options: {
projectName: project.metadata.name,
librariesPattern: !isRootProject ? "/resources/**/*.library" : undefined,
themesPattern: !isRootProject ? "/resources/sap/ui/core/themes/*" : undefined,
librariesPattern: !buildContext.isRootProject() ? "/resources/**/*.library" : undefined,
themesPattern: !buildContext.isRootProject() ? "/resources/sap/ui/core/themes/*" : undefined,
inputPattern: "/resources/**/themes/*/library.source.less"
}
});
6 changes: 3 additions & 3 deletions lib/types/themeLibrary/ThemeLibraryBuilder.js
Original file line number Diff line number Diff line change
@@ -18,7 +18,7 @@ const tasks = { // can't require index.js due to circular dependency
};

class ThemeLibraryBuilder extends AbstractBuilder {
addStandardTasks({resourceCollections, project, isRootProject, log, buildContext}) {
addStandardTasks({resourceCollections, project, log, buildContext}) {
this.addTask("replaceCopyright", () => {
const replaceCopyright = tasks.replaceCopyright;
return replaceCopyright({
@@ -48,8 +48,8 @@ class ThemeLibraryBuilder extends AbstractBuilder {
dependencies: resourceCollections.dependencies,
options: {
projectName: project.metadata.name,
librariesPattern: !isRootProject ? "/resources/**/*.library" : undefined,
themesPattern: !isRootProject ? "/resources/sap/ui/core/themes/*" : undefined,
librariesPattern: !buildContext.isRootProject() ? "/resources/**/*.library" : undefined,
themesPattern: !buildContext.isRootProject() ? "/resources/sap/ui/core/themes/*" : undefined,
inputPattern: "/resources/**/themes/*/library.source.less"
}
});
9 changes: 5 additions & 4 deletions test/lib/builder/builder.js
Original file line number Diff line number Diff line change
@@ -516,9 +516,9 @@ test("Build theme.j even without an library", (t) => {

test.serial("Cleanup", async (t) => {
const BuildContext = require("../../../lib/builder/BuildContext");
const projectContext = "project context";
const projectContext = {isRootProject: sinon.stub()};
const createProjectContextStub = sinon.stub(BuildContext.prototype, "createProjectContext").returns(projectContext);
const executeCleanupTasksStub = sinon.stub(BuildContext.prototype, "executeCleanupTasks").returns(projectContext);
const executeCleanupTasksStub = sinon.stub(BuildContext.prototype, "executeCleanupTasks").resolves();
const applicationType = require("../../../lib/types/application/applicationType");
const appBuildStub = sinon.stub(applicationType, "build").resolves();

@@ -547,10 +547,11 @@ test.serial("Cleanup", async (t) => {
t.deepEqual(appBuildStub.callCount, 1, "Build called once");
t.deepEqual(createProjectContextStub.callCount, 1, "One project context got created");
const createProjectContextParams = createProjectContextStub.getCall(0).args[0];
t.truthy(createProjectContextParams.project, "project object provided");
t.truthy(createProjectContextParams.resources.workspace, "resources.workspace object provided");
t.truthy(createProjectContextParams.resources.dependencies, "resources.dependencies object provided");
t.deepEqual(Object.keys(createProjectContextParams), ["resources"],
"resource parameter (and no others) provided");
t.deepEqual(Object.keys(createProjectContextParams), ["project", "resources"],
"resource and project parameters provided");
t.deepEqual(executeCleanupTasksStub.callCount, 1, "Cleanup called once");

// Error case
4 changes: 4 additions & 0 deletions test/lib/tasks/buildThemes.js
Original file line number Diff line number Diff line change
@@ -5,6 +5,10 @@ const mock = require("mock-require");

let buildThemes = require("../../../lib/tasks/buildThemes");

test.before(() => {
require("@ui5/logger").setLevel("verbose");
Copy link
Member

Choose a reason for hiding this comment

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

Was this intentionally added or is it a leftover from some debugging?

Copy link
Member Author

Choose a reason for hiding this comment

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

Intensionally, to also cover the verbose logging code.
Should I add a comment? I actually removed it as I thought it would be clear 😄

});

test.beforeEach((t) => {
// Stubbing processors/themeBuilder
t.context.themeBuilderStub = sinon.stub();