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

PDCL-10688 - Support custom builds using the npm package #1087

Merged
merged 22 commits into from
Apr 26, 2024
Merged

Conversation

shammowla
Copy link
Collaborator

@shammowla shammowla commented Nov 20, 2023

Overview

The customBuild.js feature enables users to create custom builds of the project by excluding optional components based on the @skipwhen directive in componentCreators.js. This allows for a more lightweight and tailored build process, excluding components not required for a specific deployment.

Custom Build Process

The custom build process involves analyzing the componentCreators.js file for @skipwhen directives. These directives indicate conditions under which components can be excluded from the build. For example, a component might be excluded if a certain environment variable is set to false.

Example Directive

/* @skipwhen ENV.alloy_personalization === false */

import createPersonalization from "../components/Personalization";

In this example, the createPersonalization component will be excluded from the build if the ENV.alloy_personalization environment variable is set to false.

Modifying customBuild.js

To support the custom build process, modifications in customBuild.js are required to parse the @skipwhen directives and exclude components accordingly.

Key Steps:

  1. Parse componentCreators.js to identify @skipwhen directives.
  2. Evaluate the conditions specified in the directives.
  3. Exclude components from the build based on those conditions.

Testing Custom Builds with runFunctionalTests.js

The runFunctionalTests.js script allows users to test custom builds by excluding tests for components not included in the build. This ensures that only relevant tests are run, aligning the testing process with the custom build.

Modifications in runFunctionalTests.js

To support testing custom builds, runFunctionalTests.js needs to be aware of which components have been excluded from the build and skip tests for those components.

Key Steps:
  1. Read the custom build configuration to determine excluded components.
  2. Adjust the tests specs glob pattern to exclude tests for these components.

Example Modification

// Exclude tests for components not included in the custom build

const excludedComponents = ["Personalization"]; // Example list of excluded components

const adjustedComponentNames = componentNames.filter(name => !excludedComponents.includes(name));

In this example, tests for the Personalization component are excluded based on the custom build configuration.

Conclusion

The customBuild.js feature, along with modifications to runFunctionalTests.js, provides a streamlined process for creating and testing custom builds. By excluding optional components and their tests, users can optimize the build and testing process for their specific needs.

@shammowla shammowla changed the title Pdcl 10688 PDCL-10688 - Support custom builds using the npm package Nov 20, 2023
@jfkhoury jfkhoury requested a review from jonsnyder November 20, 2023 18:08
Copy link
Contributor

@jonsnyder jonsnyder left a comment

Choose a reason for hiding this comment

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

I haven't been following this work too closely, so I'm a bit late to this. All of these comments could be addressed in a separate ticket.

}
}
};
};
Copy link
Contributor

Choose a reason for hiding this comment

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

One of the goals of this project is to get Adobe Tags to support this build method. To that end whatever thing we make could be used by other Tags extension developers. There is a lot of specific logic inside of the conditionalBuildBabelPlugin code that only applies to our specific use-case. For example, when I see a directive named @skipwhen it makes me think I could put this in front of anything; however, from this code it looks like you could only use it in front of an import and also if you are using the variable in an array.

I did a bit of research about conditionally building things in javascript. One of the established patterns within javascript libraries is to use an if statement that references process.env. This is especially used when referencing process.env.NODE_ENV. i.e.

if (process.env.NODE_ENV === "development") {
  console.log("development mode!");
}

I put together a sample to see if this would work with Rollup. I've also noticed that a lot of rollup systems have plugins or ways to accomplish this. https://github.com/jonsnyder/conditional-compilation-sample

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point @jonsnyder!

@shammowla Please check out Jon's sample and maybe let's schedule a work session to make this PR more reusable.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that the Babel plugin was from @dompuiu, so it may have something to do with how he envisions Launch support will happen i.e. furance/forge will run Babel but not rollup.

exportName: "createMachineLearning",
filePath: "MachineLearning"
}
];
Copy link
Contributor

Choose a reason for hiding this comment

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

Like Joe has mentioned, I don't like having to update this file anytime we create a new component. One way we could do this is import "src/core/componentCreators.js" within scripts. To do this we'd have to make sure we are using Node version >= 13 because currently scripts are written with require and the rest of the code is written with import. Ideally we should move all the code to use import and require a version of Node that supports it. Then we could loop through the component array and grab the name of each one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Deleted the componentNames.js and dynamically creating the list from componentCreators. customBuild.js script reads componentCreators.js, uses a regular expression to find all occurrences of create followed by an uppercase letter and any number of word characters (which should match the names of the component creator functions), formats the component names for export, and writes them to componentNames.js.

"identity",
"datacollector",
"libraryinfo"
];
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think any component should be "untouchable". If there is some logic that is untouchable it should be incorporated into core. Some of these components will need some work to make that a reality.

  • context - This could be removed except for the fact that it handles adding a timestamp and implementationInfo to the XDM. This functionality should be put into core.
  • privacy - I believe you can leave this out now without any problems. I see this being a big use case for customers too if they don't ever use consent.
  • identity - I can see a customer wanting to remove this if they use FPID. I think the library would work without identity right now.
  • datacollector - Currently this component defines "sendEvent" command. We should probably make "sendEvent" command part of core, and then each component can apply its own logic via onBeforeEvent lifecycle hook.
  • libraryinfo - We could make this part of core.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jonsnyder Do you think we should do this before carrying on with the custom build task?

console.log(`📏 Size: ${getFileSizeInKB(minifiedBuild.output[0].file)} KB`);
};

buildWithComponents(!!process.env.SANDBOX);
Copy link
Contributor

Choose a reason for hiding this comment

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

I can see some customers wanting an allow-list instead of an exclude-list.

@carterworks carterworks mentioned this pull request Nov 27, 2023
7 tasks
Copy link
Contributor

@carterworks carterworks left a comment

Choose a reason for hiding this comment

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

I'm mostly in agreeance with with Jon brought up.

}
}
};
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that the Babel plugin was from @dompuiu, so it may have something to do with how he envisions Launch support will happen i.e. furance/forge will run Babel but not rollup.

@dompuiu
Copy link
Member

dompuiu commented Nov 28, 2023

The babel plugin in this PR is not the same as what I had in my repo: https://github.com/dompuiu/myalloy/blob/cliBuildAll/scripts/conditionalBuildBabelPlugin.mjs.

The plugin from my repo is doing two things that I think it is generic enough:

  1. Remove any import statement that has a @skipwhen comment on the line above it;
  2. Remove any require statement with a @skipwhen comment on the line above it.

While the process.env approach is standard in the current Node ecosystem, I prefer not to tie the Launch building system to Node. There is a push to write JS builders in Rust (SWC, parcel2) or Zig (bun). Which might bring huge build speed improvements. Having a new builder for Launch libraries is important for other reasons, not just conditional compiling.

The @skipwhen comment type was agreed upon during our last meeting. Or that was the impression that I had when we left the meeting. I still think this approach is better because it allows us to implement the logic independently of each other if that's what we want. If you don't want to use the Babel plugin I wrote, you can implement the conditional logic differently. But we need a common ground from where we can start the building. If you no longer like this, we can meet again and discuss options.

@shammowla shammowla added the ignore-for-release Do not include this PR in release notes label Jan 8, 2024
Copy link
Contributor

@carterworks carterworks left a comment

Choose a reason for hiding this comment

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

Just a few tiny changes, then it looks good.

package.json Outdated
@@ -1,6 +1,6 @@
{
"name": "@adobe/alloy",
"version": "2.19.1",
"version": "2.19.0-beta.14",
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure this gets updated to at least 2.19.1

@@ -15,27 +15,37 @@ governing permissions and limitations under the License.
import createDataCollector from "../components/DataCollector";
import createActivityCollector from "../components/ActivityCollector";
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure all OPTIONAL_COMPONENTS, like createActivityCollector, have a @skipwhen comment that allows them to be excluded.

const requiredComponents = componentCreatorsContent
.match(/REQUIRED_COMPONENTS = \[[\s\S]*?\]/)[0]
.match(/create[A-Z]\w+/g)
.map(component => component.slice(6).toLowerCase());
Copy link
Contributor

Choose a reason for hiding this comment

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

What does the 6 represent? I would like to see this done in a way that doesn't have a (seemingly) random number. Same thing with the one on line 41.

@carterworks carterworks mentioned this pull request Jan 11, 2024
7 tasks
createIdentity,
createAudiences,
createPersonalization,
const REQUIRED_COMPONENTS = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment here that the conditional build relies on the naming of this variable so don't change it.

@shammowla
Copy link
Collaborator Author

Changes to runFunctionalTests.js

The runFunctionalTests.js script has been updated to read the dist/alloy.js file, check for included components, and run functional tests only for those components.

Reading dist/alloy.js

The script uses the fs.readFile function to read the dist/alloy.js file:

JavaScript

fs.readFile('dist/alloy.js', 'utf8', (err, data) => {

// ...

});

If an error occurs while reading the file, the error is logged to the console and the script returns.

Checking for Included Components

For each test file, the script extracts the component name from the file path and checks if dist/alloy.js includes alloy_${componentName}:

JavaScript

const testFiles = files.filter(file => {

const componentName = file.split('/')[3]; // assuming the file path is 'test/functional/specs/{componentName}/...'

return data.includes(alloy_${componentName});

});

If dist/alloy.js includes alloy_${componentName}, it means the component is included in the build, and the test file is added to the testFiles array.

Running Functional Tests

The script runs the functional tests for the files in the testFiles array:

JavaScript

createTestCafe().then(testcafe => {

const runner = testcafe.createRunner();

runner

.src(testFiles)

.browsers("chrome")

.run()

.then(() => testcafe.close());

});

Assumptions

This script assumes a specific directory structure for the test files: 'test/functional/specs/{componentName}/...'. If your directory structure is different, you might need to adjust the way the component name is extracted from the file path.

Usage

You can run this script with the following command:

bash

node scripts/helpers/runFunctionalTests.js

This will run the functional tests for the components included in the dist/alloy.js build.

@dompuiu dompuiu marked this pull request as ready for review April 26, 2024 19:51
@dompuiu dompuiu merged commit 936fc1a into main Apr 26, 2024
4 checks passed
@dompuiu dompuiu deleted the PDCL-10688 branch April 26, 2024 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ignore-for-release Do not include this PR in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants