Skip to content

feat: support commonAppConfig site config option #179

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

Merged
arbrandes merged 1 commit into from
Mar 16, 2026
Merged

feat: support commonAppConfig site config option #179

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
55 changes: 55 additions & 0 deletions runtime/config/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import { CONFIG_CHANGED } from '../constants';
import * as subscriptions from '../subscriptions';
import {
addAppConfigs,
getAppConfig,
getSiteConfig,
mergeSiteConfig,
setSiteConfig,
Expand Down Expand Up @@ -295,4 +297,57 @@ describe('mergeSiteConfig', () => {
expect(getSiteConfig().apps![0].config!.ORIGINAL).toBe('value');
});
});

describe('getAppConfig with commonAppConfig', () => {
it('should return commonAppConfig values for an app with no config', () => {
setSiteConfig({
...defaultSiteConfig,
commonAppConfig: { COMMON_KEY: 'common-value' },
apps: [{ appId: 'test-app' }],
});
addAppConfigs();

const config = getAppConfig('test-app');
expect(config).toEqual({ COMMON_KEY: 'common-value' });
});

it('should let app-specific config override commonAppConfig', () => {
setSiteConfig({
...defaultSiteConfig,
commonAppConfig: { SHARED: 'common', COMMON_ONLY: 'yes' },
apps: [{ appId: 'test-app', config: { SHARED: 'app-specific', APP_ONLY: 'yes' } }],
});
addAppConfigs();

const config = getAppConfig('test-app');
expect(config).toEqual({
SHARED: 'app-specific',
COMMON_ONLY: 'yes',
APP_ONLY: 'yes',
});
});

it('should return app config as-is when commonAppConfig is not set', () => {
setSiteConfig({
...defaultSiteConfig,
apps: [{ appId: 'test-app', config: { VALUE: 'test' } }],
});
addAppConfigs();

const config = getAppConfig('test-app');
expect(config).toEqual({ VALUE: 'test' });
});

it('should deep merge commonAppConfig with app config', () => {
setSiteConfig({
...defaultSiteConfig,
commonAppConfig: { NESTED: { a: 1, b: 2 } },
apps: [{ appId: 'test-app', config: { NESTED: { b: 3, c: 4 } } }],
});
addAppConfigs();

const config = getAppConfig('test-app');
expect(config).toEqual({ NESTED: { a: 1, b: 3, c: 4 } });
});
});
});
6 changes: 5 additions & 1 deletion runtime/config/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,11 @@ export function addAppConfigs() {
}

export function getAppConfig(id: string) {
return appConfigs[id];
const { commonAppConfig } = getSiteConfig();
if (commonAppConfig === undefined) {
return appConfigs[id];
}
return merge({}, commonAppConfig, appConfigs[id]);
Comment on lines +275 to +279
Copy link
Contributor

@brian-smith-tcril brian-smith-tcril Mar 12, 2026

Choose a reason for hiding this comment

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

I'm torn on if it makes sense to do this here and have the commonAppConfig merged it at read time (every time we call getAppConfig), or if we should store the merged ones.

I do like that having this here means the site config object itself holds a "cleaner" representation of the config, but calling merge on read doesn't feel great.

That being said, I can't think of a simple merge-on-write strategy for this that won't result in extra complexity or risk of outdated appConfig data.

I don't think we're calling getAppConfig directly in render loops, but that's something that would happen at the app level, not within frontend-base itself, so adding weight to this call means app developers have to think about it more than they did when it was just returning an entry from a record.

I'm happy with leaving this as-is for now and landing it, mostly just wondering what your thoughts about it are.

Copy link
Contributor Author

@arbrandes arbrandes Mar 13, 2026

Choose a reason for hiding this comment

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

getAppConfig isn't called in render loops within frontend-base. CurrentAppProvider calls it once on mount and again only when CONFIG_CHANGED fires. Downstream, useAppConfig() (which is what people should be using instead of getAppConfig) reads from React context and never calls getAppConfig directly. So the merge cost is only paid on mount and config change events, not per-render.

A merge-on-write strategy would mean merging in addAppConfigs() and re-merging all app configs whenever commonAppConfig changes via mergeSiteConfig() — plus risking stale data if commonAppConfig is mutated after addAppConfigs() runs. Merge-on-read is just simpler, IMHO.

Copy link
Contributor

@brian-smith-tcril brian-smith-tcril Mar 16, 2026

Choose a reason for hiding this comment

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

Sounds good! I just wanted to make sure both options were considered.

}

export function mergeAppConfig(id: string, newAppConfig: AppConfig) {
Expand Down
1 change: 1 addition & 0 deletions types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ export interface OptionalSiteConfig {
externalRoutes: ExternalRoute[],
externalLinkUrlOverrides: string[],
runtimeConfigJsonUrl: string | null,
commonAppConfig: AppConfig,
headerLogoImageUrl: string,

// Theme
Expand Down
Loading