diff --git a/pwa-kit b/pwa-kit new file mode 160000 index 0000000..d7fbef2 --- /dev/null +++ b/pwa-kit @@ -0,0 +1 @@ +Subproject commit d7fbef24b579a6b64514794d4e06a61c275a64b4 diff --git a/src/commands/lightning/dev/app.ts b/src/commands/lightning/dev/app.ts index 74b0f1f..0bd3d0e 100644 --- a/src/commands/lightning/dev/app.ts +++ b/src/commands/lightning/dev/app.ts @@ -51,31 +51,35 @@ class AppServerIdentityTokenService implements IdentityTokenService { if (result.success) { return result.id; } + // you might want SfError.wrap(result) here to get a more detailed error message + // for example, the server error might include more information about why the insert failed throw new Error('Could not save the token to the server'); } } +// since your commands don't return anything, you can set public static enableJsonFlag = false to keep people from using `--json` + export default class LightningDevApp extends SfCommand { public static readonly summary = messages.getMessage('summary'); public static readonly description = messages.getMessage('description'); public static readonly examples = messages.getMessages('examples'); - public static readonly flags = { name: Flags.string({ summary: messages.getMessage('flags.name.summary'), char: 'n', }), 'target-org': Flags.requiredOrg({ - summary: messages.getMessage('flags.target-org.summary'), + // if you use target-org, you probably want to use the api-version flag as well + summary: messages.getMessage('flags.target-org.summary'), // the summary isn't adding anything. I'd remove it so the standard on appears }), 'device-type': Flags.option({ summary: messages.getMessage('flags.device-type.summary'), char: 't', - options: [Platform.desktop, Platform.ios, Platform.android] as const, + options: [Platform.desktop, Platform.ios, Platform.android] as const, // enums are considered "bad" TS. Prefer a union type default: Platform.desktop, })(), 'device-id': Flags.string({ - summary: messages.getMessage('flags.device-id.summary'), + summary: messages.getMessage('flags.device-id.summary'), // are there any validations that might help people not enter these wrong if they enter them manually? char: 'i', }), }; @@ -169,14 +173,19 @@ export default class LightningDevApp extends SfCommand { let sfdxProjectRootPath = ''; try { - sfdxProjectRootPath = await SfProject.resolveProjectPath(); + sfdxProjectRootPath = await SfProject.resolveProjectPath(); // that should throw a fine error message if no project is found, no try/catch required. + // then you could just have `const sfdxProjectRootPath = await SfProject.resolveProjectPath();` and let the error bubble. + // that also ensures all the functions you pass it to don't have to re-validate that it exists, isDirectory, etc. } catch (error) { - return Promise.reject(new Error(messages.getMessage('error.no-project', [(error as Error)?.message ?? '']))); + return Promise.reject(new Error(messages.getMessage('error.no-project', [(error as Error)?.message ?? '']))); // you can throw an error without the promise wrapper } logger.debug('Configuring local web server identity'); const connection = targetOrg.getConnection(undefined); const username = connection.getUsername(); + // in reality, there will always be a username. It's typed as a ? because you can create a connection as part of saving an Org/AuthInfo. But a connection from a targetOrg will always have a username. + // for other validation scenarios, can I introduce you to `@salesforce/ts-types`? which provides handy things like + // `const username = ensureString(connection.getUsername(), messages.getMessage('error.username')` if (!username) { return Promise.reject(new Error(messages.getMessage('error.username'))); } @@ -190,6 +199,9 @@ export default class LightningDevApp extends SfCommand { // The appName is optional but if the user did provide an appName then it must be // a valid one.... meaning that it should resolve to a valid appId. + // this looks like the only consume for getAppId, so maybe that method should throw an error if no appId is found + // that would simplify your command ex: const appId = appName ? await OrgUtils.getAppId(connection, appName) : undefined; + // ensureString is also an option here appId = await OrgUtils.getAppId(connection, appName); if (!appId) { return Promise.reject(new Error(messages.getMessage('error.fetching.app-id', [appName]))); @@ -209,6 +221,7 @@ export default class LightningDevApp extends SfCommand { const entityId = await PreviewUtils.getEntityId(username); if (platform === Platform.desktop) { + // maybe these methods should take objects instead of huge # of params await this.desktopPreview(sfdxProjectRootPath, serverPorts, token, entityId, ldpServerUrl, appId, logger); } else { await this.mobilePreview( @@ -253,6 +266,7 @@ export default class LightningDevApp extends SfCommand { // OrgOpenCommand simply throws an error which will get bubbled up to LightningPreviewApp. // // Here we've chosen the second approach + // a simpler alternative would be to use the Org.getUsername() and just use the `target-org ${username}` in all 3 scenarios. const idx = this.argv.findIndex((item) => item.toLowerCase() === '-o' || item.toLowerCase() === '--target-org'); let targetOrg: string | undefined; if (idx >= 0 && idx < this.argv.length - 1) { @@ -301,12 +315,13 @@ export default class LightningDevApp extends SfCommand { // Boot the device if not already booted this.spinner.start(messages.getMessage('spinner.device.boot', [device.toString()])); + // if you overload getMobileDevice, TS could know the type of `device` based on which platform you passed in const resolvedDeviceId = platform === Platform.ios ? (device as IOSSimulatorDevice).udid : device.name; const emulatorPort = await PreviewUtils.bootMobileDevice(platform, resolvedDeviceId, logger); this.spinner.stop(); // Configure certificates for dev server secure connection - this.spinner.start(messages.getMessage('spinner.cert.gen')); + this.spinner.start(messages.getMessage('spinner.cert.gen')); // alternatively, you can use this.spinner.status = 'foo' to change what it says instead of having multiple spinners const { certData, certFilePath } = await PreviewUtils.generateSelfSignedCert(platform, sfdxProjectRootPath); this.spinner.stop(); diff --git a/src/commands/lightning/dev/site.ts b/src/commands/lightning/dev/site.ts index f62f54f..ddc6521 100644 --- a/src/commands/lightning/dev/site.ts +++ b/src/commands/lightning/dev/site.ts @@ -16,6 +16,7 @@ import { ExperienceSite } from '../../../shared/experience/expSite.js'; Messages.importMessagesDirectoryFromMetaUrl(import.meta.url); const messages = Messages.loadMessages('@salesforce/plugin-lightning-dev', 'lightning.dev.site'); +// looks like this might have meant to be a return type? export type LightningDevSiteResult = { path: string; }; @@ -24,17 +25,20 @@ export default class LightningDevSite extends SfCommand { public static readonly summary = messages.getMessage('summary'); public static readonly description = messages.getMessage('description'); public static readonly examples = messages.getMessages('examples'); + // if this isn't ready for public use, you can mark it hidden like public static hidden = true; public static readonly flags = { name: Flags.string({ summary: messages.getMessage('flags.name.summary'), char: 'n', - required: false, + required: false, // can be omitted }), debug: Flags.boolean({ + // doesn't appear to be used. Remove? summary: messages.getMessage('flags.debug.summary'), }), - 'target-org': Flags.optionalOrg({ summary: messages.getMessage('flags.target-org.summary') }), + // again, you probably want api-version flag and have it depend on target-org being present + 'target-org': Flags.optionalOrg({ summary: messages.getMessage('flags.target-org.summary') }), // the summary isn't adding anything. I'd remove it so the standard on appears }; public async run(): Promise { @@ -78,7 +82,7 @@ export default class LightningDevSite extends SfCommand { open: false, port: 3000, logLevel: 'error', - mode: 'dev', + mode: 'dev', // doesn't match the type siteZip, siteDir: selectedSite.getSiteDirectory(), }); diff --git a/src/lwc-dev-server/index.ts b/src/lwc-dev-server/index.ts index 834be5f..99826b1 100644 --- a/src/lwc-dev-server/index.ts +++ b/src/lwc-dev-server/index.ts @@ -9,7 +9,7 @@ import { existsSync, lstatSync, readFileSync } from 'node:fs'; import path from 'node:path'; import process from 'node:process'; import { LWCServer, LogLevel, ServerConfig, startLwcDevServer, Workspace } from '@lwc/lwc-dev-server'; -import { Logger } from '@salesforce/core'; +import { Logger, SfProject } from '@salesforce/core'; import { SSLCertificateData } from '@salesforce/lwc-dev-mobile-core'; import { ConfigUtils, @@ -44,6 +44,7 @@ function mapLogLevel(cliLogLevel: number): number { } async function createLWCServerConfig( + // I see y'all doing this a lot. It's easy/performant to construct child loggers like Logger.childFromRoot('createLWCServerConfig') to make it easier to trace which function is logging logger: Logger, rootDir: string, token: string, @@ -51,7 +52,7 @@ async function createLWCServerConfig( certData?: SSLCertificateData, workspace?: Workspace ): Promise { - const sfdxConfig = path.resolve(rootDir, 'sfdx-project.json'); + const sfdxConfig = path.resolve(rootDir, 'sfdx-project.json'); // you shouldn't do this. use SfProject.getInstance(rootDir) if (!existsSync(sfdxConfig) || !lstatSync(sfdxConfig).isFile()) { throw new Error(`sfdx-project.json not found in ${rootDir}`); @@ -66,7 +67,7 @@ async function createLWCServerConfig( // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access if (dir.path) { // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-argument - const resolvedDir = path.resolve(rootDir, dir.path, 'main', 'default'); + const resolvedDir = path.resolve(rootDir, dir.path, 'main', 'default'); // main/default is a convention. It's not always present if (existsSync(resolvedDir) && lstatSync(resolvedDir).isDirectory()) { logger.debug(`Adding ${resolvedDir} to namespace paths`); namespacePaths.push(resolvedDir);