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

Chore: Migrating from using the cordova-res package to using the capacitor-assets package for asset generation #2364

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Chore: Migrating from using the cordova-res package to using the capacitor-assets package for asset generation #2364

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0542ffa
chore: migrating files from the old branch to the new branch
kwAsant Aug 13, 2024
3bfbc3c
fix: Fixed the issue of the flag not working in the program when run…
kwAsant Aug 14, 2024
2fab38c
chore: updated the config file to include the for the and folders.
kwAsant Aug 14, 2024
3e441a7
chore: Ammended the file to incorporate legacy considerations within …
kwAsant Aug 14, 2024
a423a93
chore: altered code in order to align with Johnny's feedback
kwAsant Aug 20, 2024
12f4ca1
chore: Altering code to align with Johnny's feedback
kwAsant Aug 21, 2024
cf99cf6
chore: Applying advised changes
kwAsant Aug 21, 2024
c4f3033
chore: Changed comment in
kwAsant Aug 21, 2024
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
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@
"@angular/compiler": "~17.2.2",
"@angular/compiler-cli": "~17.2.2",
"@angular/language-service": "~17.2.2",
"@capacitor/assets": "^3.0.5",
"@capacitor/cli": "^5.5.1",
"@compodoc/compodoc": "^1.1.23",
"@ionic/angular-toolkit": "^10.0.0",
Expand Down
6 changes: 5 additions & 1 deletion packages/data-models/deployment.model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,9 @@ export interface IDeploymentConfig {
app_id?: string;
/** Play store app name, e.g. "Example App" */
app_name?: string;
/** Location of source android assets (splash and launcher source images). */
/** Path to source assets used to generate the app icon and splash screen. */
jfmcquade marked this conversation as resolved.
Show resolved Hide resolved
assets_path?: string;
/** LEGACT: Location of source android assets (splash and launcher source images). */
jfmcquade marked this conversation as resolved.
Show resolved Hide resolved
icon_asset_path?: string;
splash_asset_path?: string;
icon_asset_foreground_path?: string;
Expand Down Expand Up @@ -95,6 +97,8 @@ export interface IDeploymentConfig {
app_id?: string;
/** App Store app name, e.g. "Example App" */
app_name?: string;
/** Path to source assets used to generate the app icon and splash screen */
jfmcquade marked this conversation as resolved.
Show resolved Hide resolved
assets_path?: string;
};
/** 3rd party integration for remote asset storage and sync */
supabase: {
Expand Down
89 changes: 40 additions & 49 deletions packages/scripts/src/tasks/providers/android.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import { resolve, dirname, join } from "path";
import { Options, run } from "cordova-res";
import fs from "fs";
import { envReplace } from "@idemsInternational/env-replace";
import { ROOT_DIR } from "../../paths";
import { Logger, generateVersionCode } from "../../utils";
import { PATHS } from "shared";
import { execSync } from "child_process";
import * as path from "path";

interface IAndroidBuildOptions {
appId: string;
Expand Down Expand Up @@ -78,27 +78,13 @@ const set_splash_image = async (splashAssetPath: string) => {
});
}

const cordovaOptions: Options = {
directory: ROOT_DIR,
resourcesDirectory: join(ROOT_DIR, "resources"),
logstream: process.stdout,
platforms: {
android: {
splash: {
sources: [splashAssetPath],
},
},
},
skipConfig: true,
copy: true,
projectConfig: {
android: {
directory: join(ROOT_DIR, "android"),
},
},
};
// we do want it to strip the file name and use the directory
// so we can check if the directory exists
kwAsant marked this conversation as resolved.
Show resolved Hide resolved
const splashAssetDirPath = path.dirname(splashAssetPath);

return await run(cordovaOptions);
// if it does not, then otherwise, run the following command
jfmcquade marked this conversation as resolved.
Show resolved Hide resolved
const cmd = `npx @capacitor/assets generate --assetPath ${splashAssetDirPath} --android`;
execSync(cmd); // command should work, mine (Jody) is not working for some reason
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Hopefully this is working now and this comment can be removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is now 🤣

Copy link
Collaborator

Choose a reason for hiding this comment

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

If this works, why do we need the more complicated final version where the path gets manipulated to remove the ../../ and then the process gets run relative to a different directory? Or is it only that latter version that works?

Copy link
Collaborator Author

@kwAsant kwAsant Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I believe that I have removed that function. Regarding the ../../, I am not sure why it is the case. I think it has to do with how the assetPath flag works. But I have removed the code that this comment is referring too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I can see that this code is now removed. But I was thinking – if this logic (now removed) works, why not use this logic in the set_icons_and_splash_images function that remains? Is it that this logic never actually worked?

};

const set_launcher_icon = async (options: {
kwAsant marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -118,38 +104,43 @@ const set_launcher_icon = async (options: {
});
}

const includeAdaptiveIcons =
fs.existsSync(iconAssetForegroundPath) && fs.existsSync(iconAssetBackgroundPath);

const cordovaOptions: Options = {
directory: ROOT_DIR,
resourcesDirectory: join(ROOT_DIR, "resources"),
logstream: process.stdout,
platforms: {
android: includeAdaptiveIcons
? {
"adaptive-icon": {
icon: { sources: iconSources },
foreground: { sources: [iconAssetForegroundPath] },
background: { sources: [iconAssetBackgroundPath] },
},
}
: { icon: { sources: iconSources } },
},
skipConfig: true,
copy: true,
projectConfig: {
android: {
directory: join(ROOT_DIR, "android"),
},
},
};
// all paths for the icons have the same diretory
const iconAssetDirPath = path.dirname(iconAssetPath);
const cmd = `npx @capacitor/assets generate --assetPath ${iconAssetDirPath} --android`;
execSync(cmd);

console.log(cmd);
};

const set_icons_and_splash_images = async (options: { assetPath: string }) => {
const { assetPath } = options;

const iconAndSplashSources = [];
if (fs.existsSync(assetPath)) {
iconAndSplashSources.push(assetPath);
} else {
return Logger.error({
msg1: "Icon and splash source assets not found",
msg2: `A source .png file is required to be used as a fall back for when the device's android version does not support adaptive icons. No asset was found at the path supplied in the deployment config: ${assetPath}.`,
});
}

// make a check for the direo

// all paths for the icons have the same diretory
const assetDirPath = assetPath.includes(".png") ? path.dirname(assetPath) : assetPath;
Copy link
Collaborator

@jfmcquade jfmcquade Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Do we know whether the file has to be a .png? Either way, I think it would be better to check whether the path is to a file in a more general way. Have a look at these docs (we're already importing fs so can use its methods)

const relativeAssetPath = path
.relative(process.cwd(), assetDirPath)
.replace(/^(\.\.\/|\.\/)+/, "");
const cmd = `npx @capacitor/assets generate --assetPath ${relativeAssetPath} --android`;
const cwd = process.cwd().replace("/packages/scripts", ""); // output will be: "/../../idems/open-app-builder/packages/scripts"

return await run(cordovaOptions);
execSync(cmd, { stdio: "inherit", cwd });
};

export default {
configure,
set_launcher_icon,
set_splash_image,
set_icons_and_splash_images,
};
26 changes: 26 additions & 0 deletions packages/scripts/src/tasks/providers/ios.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@ import { envReplace } from "@idemsInternational/env-replace";
import { Logger, generateVersionCode } from "../../utils";
import { PATHS } from "shared";

import fs from "fs";
import { execSync } from "child_process";
import * as path from "path";

interface IiOSBuildOptions {
appId: string;
appName: string;
Expand Down Expand Up @@ -45,6 +49,27 @@ const configure = async ({ appId, appName, versionName }: IiOSBuildOptions) => {
});
};

const set_icons_and_splash_images = async (options: { assetPath: string }) => {
const { assetPath } = options;

const iconAndSplashSources = [];
if (fs.existsSync(assetPath)) {
iconAndSplashSources.push(assetPath);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this accomplishing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is within the methods for android.ts. I believe they check if the folder to the assets actually exists

Copy link
Collaborator

@jfmcquade jfmcquade Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Have a look at this line again. iconAndSplashSources isn't being consumed anywhere, so what does pushing a value to it accomplish? I think you can replace lines 55-63 with something like:

if (!fs.existsSync(assetPath)) return Logger.error({
  ...etc
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not entirely sure what this one is supposed to accomplish really. It was here during the Cordova options, so I just pasted it in the new function.
I have made your changes though.

} else {
return Logger.error({
msg1: "Icon and splash source assets not found",
msg2: `A source .png file is required to be used as a fall back for when the device's android version does not support adaptive icons. No asset was found at the path supplied in the deployment config: ${assetPath}.`,
jfmcquade marked this conversation as resolved.
Show resolved Hide resolved
});
}

const relativeAssetPath = path.relative(process.cwd(), assetPath).replace(/^(\.\.\/|\.\/)+/, "");
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a nicer way to do this, without the .replace() and regex? Potentially an existing method of path? It could be worth thinking if this whole logic of manipulating the path and then then executing the command relative to the cwd could be simplified (I'm not saying that it definitely can, but it's quite hard to follow currently)

Copy link
Collaborator

Choose a reason for hiding this comment

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

See comment on line 68 below. I think this line could also use the ROOT_DIR value instead of process.cwd(), and avoid doing the regex replacement. Try logging out ROOT_DIR and assetPath at this point, and see if there's a method(s) of path that could be used to combine them and give the output you want

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Made the change here too.


const cmd = `npx @capacitor/assets generate --assetPath ${relativeAssetPath} --ios`;
const cwd = process.cwd().replace("/packages/scripts", ""); // output will be: "/../../idems/open-app-builder/packages/scripts"
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Rather than doing a string replace, it would probably be better to use the path npm library to get the correct path – path.relative(from, to) might be what you need

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have found out that only relative paths work for the npx command, so I am attempting to find methods of converting the path to a relative one (without the /../..)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With research ( and help with ChatGPT ), this may be the best way to get rid of leading dots in a directory.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK. Line 68 is effectively just getting the relative path to the root directory, right? I think you can delete this line and instead import ROOT_DIR from packages/scripts/src/paths.ts (look at the file changes for this PR, you'll see that packages/scripts/src/tasks/providers/android.ts used to import ROOT_DIR). Then you should be able to just pass this value to the execSync() function below, like:

execSync(cmd, { stdio: "inherit", cwd: ROOT_DIR });

See also comment on line 65

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have made these changes, and they function the same.


execSync(cmd, { stdio: "inherit", cwd });
};

/**
* iOS app ID (aka "bundle ID") only supports alphanumeric characters (A–Z, a–z, and 0–9), hyphens (-), and periods (.),
* see https://developer.apple.com/documentation/bundleresources/information_property_list/cfbundleidentifier#discussion
Expand All @@ -56,4 +81,5 @@ function convertToValidIOSAppId(appId: string) {

export default {
configure,
set_icons_and_splash_images,
};
23 changes: 23 additions & 0 deletions packages/workflows/src/android.workflows.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ const childWorkflows: IDeploymentWorkflows = {
// Generate Android assets from source images (splash.png, icon.png and, optionally, icon-foreground.png and icon-background.png)
// Icon images must be at least 1024×1024px, splash image must be at least 2732×2732px
// Further specifications provided here: https://www.npmjs.com/package/cordova-res
// Cordova-res is considered legacy, so we now are migrating to using capacitor-assets instead
// Further specification here: https://capacitorjs.com/docs/guides/splash-screens-and-icons
kwAsant marked this conversation as resolved.
Show resolved Hide resolved
set_splash_image: {
label: "Generate splash screen image from splash.png asset and copy to relevant folders",
steps: [
Expand All @@ -47,6 +49,19 @@ const childWorkflows: IDeploymentWorkflows = {
},
],
},
set_icons_and_splash_images: {
label:
"Generate app launcher icon and splash screen image from icon.png and splash.png assets respectively. Copy generated files to relevant folders",
steps: [
{
name: "set_icons_and_splash_images",
function: async ({ tasks, config }) =>
tasks.android.set_icons_and_splash_images({
assetPath: config.android.assets_path || config.android.icon_asset_path,
}),
},
],
},
};

/** Default workflows made available to all deployments */
Expand All @@ -71,6 +86,14 @@ const defaultWorkflows: IDeploymentWorkflows = {
function: async ({ tasks, workflow }) =>
await tasks.workflow.runWorkflow({ name: "android set_launcher_icon", parent: workflow }),
},
{
name: "Set Icons and Splash Images",
function: async ({ tasks, workflow }) =>
await tasks.workflow.runWorkflow({
name: "android set_icons_and_splash_images",
parent: workflow,
}),
},
],
children: childWorkflows,
},
Expand Down
22 changes: 22 additions & 0 deletions packages/workflows/src/ios.workflows.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,20 @@ const childWorkflows: IDeploymentWorkflows = {
},
],
},
set_icons_and_splash_images: {
label:
"Generate app launcher icon and splash screen image from icon.png and splash.png asset respectively. Then if provided, it will generate adaptive icon from icon-foreground.png and icon-background.png. Copy generated files to relevant folders.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think again that this should be "icon-only.png" instead of "icon.png". And no harm in linking to the docs again... Also copy over this comment to corresponding Android equivalent

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this one may have to be changed within the gdrive. The comment I have changed but the actual name itself might have to be changed.

steps: [
{
name: "set_icons_and_splash_images",
function: async ({ tasks, config }) =>
tasks.ios.set_icons_and_splash_images({ assetPath: config.ios.assets_path }),
},
],
},
// Generate iOS assets from source images (splash.png, icon.png and, optionally, icon-foreground.png and icon-background.png)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think icon.png should be icon-only.png instead

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not too sure what this comment is specifically referencing. When I checked my assets, the image was called icon.png. However, I think any name changes can be solved by accessing the Google Drive.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was going off the docs, which suggest that the source asset should be called "icon-only.png". So I think that's what this comment should specify as the name for the source file too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay will do

// Icon images must be at least 1024×1024px, splash image must be at least 2732×2732px
// Further specification here: https://capacitorjs.com/docs/guides/splash-screens-and-icons
};

/** Default workflows made available to all deployments */
Expand All @@ -32,6 +46,14 @@ const defaultWorkflows: IDeploymentWorkflows = {
function: async ({ tasks, workflow }) =>
await tasks.workflow.runWorkflow({ name: "ios configure", parent: workflow }),
},
{
name: "Set Icons and Splash Images",
function: async ({ tasks, workflow }) =>
await tasks.workflow.runWorkflow({
name: "ios set_icons_and_splash_images",
parent: workflow,
}),
},
],
children: childWorkflows,
},
Expand Down
Loading
Loading