Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
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
12 changes: 12 additions & 0 deletions pnpm-workspace.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,18 @@ minimumReleaseAgeExclude:
overrides:
tar: 7.5.15

supportedArchitectures:
Comment thread
l2ysho marked this conversation as resolved.
os:
- linux
- darwin
- win32
cpu:
- x64
- arm64
libc:
- glibc
- musl

# Unfortunately, several parts of this project expect everything to be hoisted up in the node_modules directory...
# And fixing it would take longer than just enabling this and shedding a tear
nodeLinker: hoisted
Expand Down
52 changes: 42 additions & 10 deletions scripts/build-cli-bundles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,28 @@ const entryPoints = [
fileURLToPath(new URL('../src/entrypoints/actor.ts', import.meta.url)),
];

// Placeholder specifier that `credentials.ts` imports for the OS keyring in bundle mode.
// Kept external in the fat-JS step so the literal `import()` survives, then rewritten per
// target below to the matching `@napi-rs/keyring-<platform>` subpackage so Bun's `--compile`
// embeds that one native `.node`. Must match the specifier in `src/lib/credentials.ts`.
const KEYRING_PLACEHOLDER = '__APIFY_KEYRING_NATIVE_SUBPACKAGE__';

// Maps the compiled (os, arch, libc) to the napi-rs keyring subpackage that ships its `.node`.
// `pnpm.supportedArchitectures` (package.json) forces all of these into node_modules at build
Comment thread
l2ysho marked this conversation as resolved.
Outdated
// time so each target can resolve its own, regardless of the build machine's platform.
function keyringSubpackage(os: string, arch: string, musl: boolean): string {
switch (os) {
case 'linux':
return `@napi-rs/keyring-linux-${arch}-${musl ? 'musl' : 'gnu'}`;
case 'darwin':
return `@napi-rs/keyring-darwin-${arch}`;
case 'windows':
return `@napi-rs/keyring-win32-${arch}-msvc`;
default:
throw new Error(`No @napi-rs/keyring subpackage known for ${os}-${arch}`);
}
}

Comment on lines +75 to +87

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is the main hack, --compile (bun) skipped napi-rs due to its dynamic require shenanigans -> we provide literal for specific bundle which is shipped as external subpackage (per arch).

https://bun.com/docs/bundler/executables

await rm(new URL('../bundles/', import.meta.url), { recursive: true, force: true });

// #region Inject the fact the CLI is ran in a bundle, instead of installed through npm/volta
Expand Down Expand Up @@ -92,21 +114,23 @@ for (const entryPoint of entryPoints) {
conditions: 'node',
target: 'bun',
sourcemap: 'none',
// Keep the keyring placeholder literal `import()` intact so it can be rewritten and
// resolved per target in step 2 (Bun only embeds a `.node` when --compile resolves it).
external: [KEYRING_PLACEHOLDER],
});

const entrypointResultFilePath = result.outputs[0]!.path;

// Fix apify client js (it now lazy loads proxy-agent, which makes bun skip it from the bundle)
{
const entrypointResultFileContent = await result.outputs[0]!.text();
// Fix apify client js (it now lazy loads proxy-agent, which makes bun skip it from the bundle).
// Kept in memory only — the per-target write below is what lands on disk before each compile.
const fatEntrypointContent = (await result.outputs[0]!.text()).replace(
`(0, utils_1.dynamicNodeImport)("proxy-agent")`,
`Promise.resolve().then(() => import_proxy_agent)`,
);

const newEntrypointResultFileContent = entrypointResultFileContent.replace(
`(0, utils_1.dynamicNodeImport)("proxy-agent")`,
`Promise.resolve().then(() => import_proxy_agent)`,
);

await writeFile(entrypointResultFilePath, newEntrypointResultFileContent);
}
// The on-disk fat JS only varies by keyring subpackage, so we rewrite it once per subpackage
// rather than once per target (baseline variants share their sibling's subpackage).
let writtenSubpackage: string | undefined;

for (const target of targets) {
// eslint-disable-next-line prefer-const -- somehow it cannot tell that os and arch cannot be "const" while the rest are let
Expand Down Expand Up @@ -141,6 +165,14 @@ for (const entryPoint of entryPoints) {

console.log(`Building ${cliName} for ${target} (result: ${fileName})...`);

// Point the keyring import at this target's native subpackage so --compile embeds its
// `.node`. Skip the rewrite when the on-disk file already targets this subpackage.
const subpackage = keyringSubpackage(os, arch, Boolean(musl));
if (subpackage !== writtenSubpackage) {
await writeFile(entrypointResultFilePath, fatEntrypointContent.replaceAll(KEYRING_PLACEHOLDER, subpackage));
Comment thread
DaveHanns marked this conversation as resolved.
Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Important: if, for any reason, the if (subpackage !== writtenSubpackage) { never passes, the proxy-agent patch at line 126 is now dropped (never written) as well.

Should not happen, AFAIK, but good to keep in mind and maybe future-proof.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

good catch 👍

writtenSubpackage = subpackage;
}

// Step 2: create the final executable bundle
await build({
entrypoints: [entrypointResultFilePath],
Expand Down
4 changes: 0 additions & 4 deletions src/commands/auth/login.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,6 @@ const tryToLogin = async (token: string) => {
tokenLocation = 'your OS keyring';
} else if (process.env.APIFY_DISABLE_KEYRING === '1') {
tokenLocation = `${AUTH_FILE_PATH()} (OS keyring disabled via APIFY_DISABLE_KEYRING)`;
} else if (process.env.APIFY_CLI_BUNDLE) {
// Bundle distributions ship without the OS keyring native module — see
// https://github.com/apify/apify-cli/issues for the tracking issue.
tokenLocation = `${AUTH_FILE_PATH()} (OS keyring not available in bundle installs; install via npm for keyring storage, or set APIFY_DISABLE_KEYRING=1 to silence)`;
} else {
tokenLocation = `${AUTH_FILE_PATH()} (OS keyring unavailable; set APIFY_DISABLE_KEYRING=1 to silence)`;
}
Expand Down
28 changes: 25 additions & 3 deletions src/lib/credentials.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import process from 'node:process';

import { AUTH_FILE_PATH } from './consts.js';
import { ensureApifyDirectory } from './files.js';
import { useCLIMetadata } from './hooks/useCLIMetadata.js';
import { cliDebugPrint } from './utils/cliDebugPrint.js';

const KEYRING_SERVICE = 'com.apify.cli';
Expand Down Expand Up @@ -41,15 +42,36 @@ export function __resetCredentialsForTests() {

async function loadKeyringModule(): Promise<KeyringModule | null> {
if (cachedKeyringModule !== undefined) return cachedKeyringModule;
cachedKeyringModule = await importKeyringModule();
return cachedKeyringModule;
}

async function importKeyringModule(): Promise<KeyringModule | null> {
// Bundle distributions can't load the `@napi-rs/keyring` wrapper: its createRequire-based
// platform loader isn't followed by Bun's `--compile`, so the native module never makes it
// into the binary. Instead each bundle embeds exactly one platform subpackage, and the
// specifier below is rewritten to it at build time (see scripts/build-cli-bundles.ts).
if (useCLIMetadata().installMethod === 'bundle') {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Q: in detectInstallMethod, we first check if (process.env.APIFY_CLI_MARKED_INSTALL_METHOD) {.

Once we publish bundled versions to NPM, the APIFY_CLI_MARKED_INSTALL_METHOD will be npm, but the underlying source will be bundle, right?

That will break this check AFAIK 🤔

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

bundles are not shiped to npm, install.sh download them from release artefacts -> https://github.com/apify/apify-cli/releases/tag/v1.6.2

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, now they are not shipped through npm, but, and correct me if I'm wrong, there either was or still is a plan to ship bundles through npm instead of plain JS.

At least the comment above following check in function detectInstallMethod() indicates so:

	// Will be useful once npm versions move to running from bundles (and for testing)
	if (process.env.APIFY_CLI_MARKED_INSTALL_METHOD) {
		return process.env.APIFY_CLI_MARKED_INSTALL_METHOD as InstallMethod;
	}

In such case, the APIFY_CLI_MARKED_INSTALL_METHOD would get set to npm even tho a bundle would be shipped and the affected if (useCLIMetadata().installMethod === 'bundle') { statement would not pass. We would then treat bundle as a plain JS, not applying the workaround this PR introduces.

That being said, I think this is not an issue as the APIFY_CLI_MARKED_INSTALL_METHOD is currently not used / set anywhere really. Let's just get rid of the check in function detectInstallMethod(). We can re-introduce it once it is needed, if ever.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes I see your point now, lets remove that.

try {
const mod = (await import('__APIFY_KEYRING_NATIVE_SUBPACKAGE__')) as Partial<KeyringModule> & {
default?: Partial<KeyringModule>;
};
const Entry = mod.Entry ?? mod.default?.Entry;
return Entry ? { Entry } : null;
} catch (err) {
cliDebugPrint('credentials', 'failed to load bundled keyring', err);
return null;
}
}

try {
// Indirect specifier so tsc doesn't try to resolve the module at compile time.
const specifier = '@napi-rs/keyring';
cachedKeyringModule = (await import(specifier)) as KeyringModule;
return (await import(specifier)) as KeyringModule;
} catch (err) {
cliDebugPrint('credentials', 'failed to load @napi-rs/keyring', err);
cachedKeyringModule = null;
return null;
}
return cachedKeyringModule;
}

/**
Expand Down
12 changes: 12 additions & 0 deletions src/lib/typings/keyring-native-subpackage.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// The specifier below is a build-time placeholder. `scripts/build-cli-bundles.ts`
// rewrites it to the platform-specific `@napi-rs/keyring-<target>` subpackage for each
// compiled bundle, so Bun's `--compile` embeds that one native `.node`. Outside bundles
// the import is never executed. This declaration just keeps tsc/oxlint happy.
declare module '__APIFY_KEYRING_NATIVE_SUBPACKAGE__' {
export class Entry {
constructor(service: string, account: string);
getPassword(): string | null;
setPassword(password: string): void;
deletePassword(): boolean;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: @napi-rs/keyring exports Entry. Re-export should be possible and avoid type drift (if the upstream Entry in @napi-rs/keyring ever change).

Suggested change
export class Entry {
constructor(service: string, account: string);
getPassword(): string | null;
setPassword(password: string): void;
deletePassword(): boolean;
}
export { Entry } from '@napi-rs/keyring';

@l2ysho l2ysho Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this is intentional, the placeholder resolves to the @napi-rs/keyring- native subpackage, which ships no .d.ts

I will update that commentary about this information.

@DaveHanns DaveHanns Jun 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, my point was that the '@napi-rs/keyring' exports Entry and it should be possible to simply re-export it from here as a type without having to "mirror" the Entry with hand written class 🤔 That way, if the Entry ever changes upstream, we get the update for free.

So, we keep the keyring-native-subpackage.d.ts, but its content would be just:

export type { Entry } from '@napi-rs/keyring';

or

export { Entry } from '@napi-rs/keyring';

Should be alright, AFAIK. Not tested tho.
That being said, it is just a nit, so feel free to ignore.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I remember I was fighting any in some point, but I tried reexport now and all 🟢

}
Loading