Skip to content
This repository has been archived by the owner on May 3, 2024. It is now read-only.

fix(load-modules): unnecessary calls to updateModuleRegistry #939

Merged
merged 2 commits into from
Mar 7, 2023
Merged

fix(load-modules): unnecessary calls to updateModuleRegistry #939

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
17a3b4d
fix(load-modules): unnecessary calls to updateModuleRegistry
JAdshead Mar 3, 2023
7aa5dcd
test(integration): update to ensure module map reset)
JAdshead Mar 7, 2023
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
23 changes: 16 additions & 7 deletions __tests__/integration/one-app.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,17 @@ describe('Tests that require Docker setup', () => {
let browser;
const moduleName = 'unhealthy-frank';
const version = '0.0.0';
const revertErrorMatch = /There was an error loading module (?<moduleName>.*) at (?<url>.*). Ignoring (?<workingModule>.*) until .*/;
let requiredExternalsError;

beforeAll(async () => {
originalModuleMap = readModuleMap();

await addModuleToModuleMap({
moduleName,
version,
});
requiredExternalsError = searchForNextLogMatch(revertErrorMatch);
({ browser } = await setUpTestRunner({ oneAppLocalPortToUse, oneAppMetricsLocalPortToUse }));
});

Expand All @@ -74,8 +78,6 @@ describe('Tests that require Docker setup', () => {
writeModuleMap(originalModuleMap);
});
test('one-app starts up successfully with a bad module', async () => {
const revertErrorMatch = /There was an error loading module (?<moduleName>.*) at (?<url>.*). Ignoring (?<workingModule>.*) until .*/;
const requiredExternalsError = searchForNextLogMatch(revertErrorMatch);
const loggedError = await requiredExternalsError;
const [, problemModule, problemModuleUrl, workingUrl] = revertErrorMatch.exec(loggedError);
const gitSha = await retrieveGitSha();
Expand Down Expand Up @@ -299,7 +301,7 @@ describe('Tests that require Docker setup', () => {
});

describe('module removed from module map', () => {
afterAll(() => {
afterAll(async () => {
const integrityDigests = retrieveModuleIntegrityDigests({
moduleName: 'healthy-frank',
version: sampleModuleVersion,
Expand All @@ -309,6 +311,7 @@ describe('Tests that require Docker setup', () => {
version: sampleModuleVersion,
integrityDigests,
});
await waitFor(5000);
});

test('removes module from one-app', async () => {
Expand All @@ -332,7 +335,10 @@ describe('Tests that require Docker setup', () => {
});

describe('new module added to module map', () => {
afterAll(() => removeModuleFromModuleMap('late-frank'));
afterAll(async () => {
removeModuleFromModuleMap('late-frank');
await waitFor(5000);
});

test('loads new module when module map updated', async () => {
await browser.url(`${appAtTestUrls.browserUrl}/demo/late-frank`);
Expand Down Expand Up @@ -373,7 +379,7 @@ describe('Tests that require Docker setup', () => {
let failedRootModuleConfigSearch;
const failedRootModuleConfig = /Root module attempted to set the following non-overrideable options for the client but not the server:\\n\s{2}someApiUrl/;

beforeEach(async () => {
beforeAll(async () => {
const nextVersion = '0.0.1';
failedRootModuleConfigSearch = searchForNextLogMatch(failedRootModuleConfig);
await addModuleToModuleMap({
Expand All @@ -387,8 +393,9 @@ describe('Tests that require Docker setup', () => {
await waitFor(5000);
});

afterEach(async () => {
afterAll(async () => {
writeModuleMap(originalModuleMap);
await waitFor(5000);
});

test('writes an error to log when failed module config', async () => {
Expand Down Expand Up @@ -432,6 +439,7 @@ describe('Tests that require Docker setup', () => {

afterEach(async () => {
writeModuleMap(originalModuleMap);
await waitFor(5000);
});

test('writes an error to log when failed child module validation', async () => {
Expand Down Expand Up @@ -778,8 +786,9 @@ describe('Tests that require Docker setup', () => {
const moduleName = 'cultured-frankie';
const version = '0.0.1';

afterEach(() => {
afterEach(async () => {
writeModuleMap(originalModuleMap);
await waitFor(5000);
});

test('fails to get external `react-intl` for child module as an unsupplied `requiredExternal` - logs failure', async () => {
Expand Down
8 changes: 4 additions & 4 deletions __tests__/server/utils/__snapshots__/loadModules.spec.jsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,21 +16,21 @@ exports[`loadModules updates the client module map cache 1`] = `
"browser": {
"modules": {
"some-root": {
"baseUrl": "https://example.com/cdn/some-root/2.2.2/",
"baseUrl": "https://example.com/cdn/some-root/1.1.2/",
"browser": {
"integrity": "nggdfhr34",
"url": "https://example.com/cdn/some-root/2.2.2/some-root.browser.js",
"url": "https://example.com/cdn/some-root/1.1.2/some-root.browser.js",
},
},
},
},
"legacyBrowser": {
"modules": {
"some-root": {
"baseUrl": "https://example.com/cdn/some-root/2.2.2/",
"baseUrl": "https://example.com/cdn/some-root/1.1.2/",
"legacyBrowser": {
"integrity": "7567ee",
"url": "https://example.com/cdn/some-root/2.2.2/some-root.legacy.browser.js",
"url": "https://example.com/cdn/some-root/1.1.2/some-root.legacy.browser.js",
},
},
},
Expand Down
101 changes: 66 additions & 35 deletions __tests__/server/utils/loadModules.spec.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,47 +44,36 @@ jest.mock('../../../src/server/plugins/csp', () => ({

const RootModule = () => ({});

describe('loadModules', () => {
const moduleMap = {
modules: {
'some-root': {
node: {
url: 'https://example.com/cdn/some-root/2.2.2/some-root.node.js',
integrity: '4y45hr',
},
browser: {
url: 'https://example.com/cdn/some-root/2.2.2/some-root.browser.js',
integrity: 'nggdfhr34',
},
legacyBrowser: {
url: 'https://example.com/cdn/some-root/2.2.2/some-root.legacy.browser.js',
integrity: '7567ee',
},
let modules;
let fetchedModuleMap;
function setFetchedRootModuleVersion(version) {
modules = {
'some-root': {
node: {
url: `https://example.com/cdn/some-root/${version}/some-root.node.js`,
integrity: '4y45hr',
},
browser: {
url: `https://example.com/cdn/some-root/${version}/some-root.browser.js`,
integrity: 'nggdfhr34',
},
legacyBrowser: {
url: `https://example.com/cdn/some-root/${version}/some-root.legacy.browser.js`,
integrity: '7567ee',
},
},
};

fetchedModuleMap = { modules };
}

describe('loadModules', () => {
beforeAll(() => {
getModule.mockImplementation(() => RootModule);
updateModuleRegistry.mockImplementation(() => ({
'some-root': {
node: {
url: 'https://example.com/cdn/some-root/2.2.2/some-root.node.js',
integrity: '4y45hr',
},
browser: {
url: 'https://example.com/cdn/some-root/2.2.2/some-root.browser.js',
integrity: 'nggdfhr34',
},
legacyBrowser: {
url: 'https://example.com/cdn/some-root/2.2.2/some-root.legacy.browser.js',
integrity: '7567ee',
},
},
}));
updateModuleRegistry.mockImplementation(() => modules);

global.fetch = jest.fn(() => Promise.resolve({
json: () => Promise.resolve(moduleMap),
json: () => Promise.resolve(fetchedModuleMap),
}));
});

Expand All @@ -94,21 +83,45 @@ describe('loadModules', () => {
});

it('updates the holocron module registry', async () => {
setFetchedRootModuleVersion('1.1.1');
await loadModules();
expect(updateModuleRegistry).toHaveBeenCalledWith({
moduleMap: addBaseUrlToModuleMap(moduleMap),
moduleMap: addBaseUrlToModuleMap(fetchedModuleMap),
batchModulesToUpdate: require('../../../src/server/utils/batchModulesToUpdate').default,
getModulesToUpdate: require('../../../src/server/utils/getModulesToUpdate').default,
onModuleLoad: require('../../../src/server/utils/onModuleLoad').default,
});
});

it('updates the client module map cache', async () => {
setFetchedRootModuleVersion('1.1.2');
await loadModules();
expect(getClientModuleMapCache()).toMatchSnapshot();
});

it('returns loaded modules', async () => {
setFetchedRootModuleVersion('2.0.0');
const loadedModules = await loadModules();
expect(loadedModules).toEqual({
'some-root': {
browser: {
integrity: 'nggdfhr34',
url: 'https://example.com/cdn/some-root/2.0.0/some-root.browser.js',
},
legacyBrowser: {
integrity: '7567ee',
url: 'https://example.com/cdn/some-root/2.0.0/some-root.legacy.browser.js',
},
node: {
integrity: '4y45hr',
url: 'https://example.com/cdn/some-root/2.0.0/some-root.node.js',
},
},
});
});

it('doesnt update caches when there are no changes', async () => {
setFetchedRootModuleVersion('1.1.3');
updateModuleRegistry.mockImplementationOnce(() => ({}));
await loadModules();
expect(getClientModuleMapCache()).toMatchSnapshot();
Expand All @@ -118,12 +131,13 @@ describe('loadModules', () => {
RootModule[CONFIGURATION_KEY] = {
csp: "default-src 'none';",
};

setFetchedRootModuleVersion('1.1.4');
await loadModules();
expect(updateCSP).toHaveBeenCalledWith("default-src 'none';");
});

it('calls updateCSP even when csp is not set', async () => {
setFetchedRootModuleVersion('1.1.5');
delete RootModule[CONFIGURATION_KEY].csp;
await loadModules();
expect(updateCSP).toHaveBeenCalledWith(undefined);
Expand All @@ -150,8 +164,25 @@ describe('loadModules', () => {
});

it('does not attempt to update CSP', async () => {
setFetchedRootModuleVersion('1.1.6');
await loadModules();
expect(updateCSP).not.toHaveBeenCalled();
});
});

describe('when module map does not change', () => {
beforeAll(async () => {
setFetchedRootModuleVersion('1.1.1');
await loadModules();
jest.clearAllMocks();
});
it('does not updateModuleRegistry', async () => {
await loadModules();
expect(updateModuleRegistry).not.toHaveBeenCalled();
});
it('does not return any modules', async () => {
const loadedModules = await loadModules();
expect(loadedModules).toEqual({});
});
});
});
24 changes: 12 additions & 12 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

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 @@ -117,6 +117,7 @@
"lean-intl": "^4.2.2",
"matcher": "^4.0.0",
"node-fetch": "^2.6.7",
"object-hash": "^3.0.0",
"opossum": "^7.1.0",
"opossum-prometheus": "^0.3.0",
"pidusage": "^3.0.2",
Expand Down
9 changes: 9 additions & 0 deletions src/server/utils/loadModules.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import { getModule } from 'holocron';
import { updateModuleRegistry } from 'holocron/server';

import hash from 'object-hash';
import onModuleLoad, { CONFIGURATION_KEY } from './onModuleLoad';
import batchModulesToUpdate from './batchModulesToUpdate';
import getModulesToUpdate from './getModulesToUpdate';
Expand All @@ -25,9 +26,17 @@ import { setClientModuleMapCache } from './clientModuleMapCache';
import { updateCSP } from '../plugins/csp';
import addBaseUrlToModuleMap from './addBaseUrlToModuleMap';

let cachedModuleMapHash;

const loadModules = async () => {
const moduleMapResponse = await fetch(process.env.HOLOCRON_MODULE_MAP_URL);
const moduleMap = addBaseUrlToModuleMap(await moduleMapResponse.json());

const moduleMapHash = hash(moduleMap);
if (cachedModuleMapHash && cachedModuleMapHash === moduleMapHash) {
return {};
Copy link
Member

Choose a reason for hiding this comment

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

Is there value in logging here, maybe in the non-cache-hit case? so those who watch logs can see '3 modules change since last poll' and have confirmation that the change has actually been processed?

Copy link
Member

Choose a reason for hiding this comment

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

We already log that

one-app/src/server/utils/pollModuleMap.js

Line 151 in 531958f

`pollModuleMap: ${numberOfModulesLoaded} modules loaded/updated:`,

}
cachedModuleMapHash = moduleMapHash;
const serverConfig = getServerStateConfig();

const loadedModules = await updateModuleRegistry({
Expand Down