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

fix: defer loading of head.html to allow inclusion of request cookie #2275

Merged
merged 2 commits into from
Nov 16, 2023
Merged

fix: defer loading of head.html to allow inclusion of request cookie #2275

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
926aff5
fix: defer loading of head.html to allow inclusion of request cookie
tripodsan Nov 15, 2023
2b87cbf
chore: increase test coverage
tripodsan Nov 15, 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
21 changes: 20 additions & 1 deletion src/server/HeadHtmlSupport.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ export default class HeadHtmlSupport {
this.remoteStatus = 0;
this.localHtml = '';
this.localStatus = 0;
this.cookie = '';
this.url = new URL(proxyUrl);
this.url.pathname = '/head.html';
this.filePath = resolve(directory, 'head.html');
Expand All @@ -89,8 +90,13 @@ export default class HeadHtmlSupport {

async loadRemote() {
// load head from server
const headers = {};
if (this.cookie) {
headers.cookie = this.cookie;
}
const resp = await getFetch()(this.url, {
cache: 'no-store',
headers,
});
this.remoteStatus = resp.status;
if (resp.ok) {
Expand All @@ -103,6 +109,18 @@ export default class HeadHtmlSupport {
}
}

setCookie(cookie) {
if (this.cookie !== cookie) {
this.log.trace('registering head-html cookie to ', cookie);
this.cookie = cookie;
this.remoteStatus = 0;
}
}

invalidateLocal() {
this.localStatus = 0;
}

async loadLocal() {
try {
this.localHtml = (await fs.readFile(this.filePath, 'utf-8')).trim();
Expand All @@ -114,7 +132,7 @@ export default class HeadHtmlSupport {
}
}

async init() {
async update() {
if (!this.localStatus) {
await this.loadLocal();
}
Expand All @@ -127,6 +145,7 @@ export default class HeadHtmlSupport {
}

async replace(source) {
await this.update();
if (!this.isModified) {
this.log.trace('head.html ignored: not modified locally.');
return source;
Expand Down
4 changes: 1 addition & 3 deletions src/server/HelixProject.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,15 +78,13 @@
log: this.log,
proxyUrl: this.proxyUrl,
});
await this._headHtml.init();

// register local head in live-reload
if (this.liveReload) {
this.liveReload.registerFiles([this._headHtml.filePath], '/');
this.liveReload.on('modified', async (modified) => {
if (modified.indexOf('/') >= 0) {
await this._headHtml.loadLocal();
await this._headHtml.init();
this._headHtml.invalidateLocal();

Check warning on line 87 in src/server/HelixProject.js

View check run for this annotation

Codecov / codecov/patch

src/server/HelixProject.js#L87 

Added line #L87 was not covered by tests
}
});
}
Expand Down
3 changes: 2 additions & 1 deletion src/server/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ window.LiveReloadOptions = {

const isHTML = ret.status === 200 && contentType.indexOf('text/html') === 0;
const livereload = isHTML && opts.injectLiveReload;
const replaceHead = isHTML && opts.headHtml && opts.headHtml.isModified;
const replaceHead = isHTML && opts.headHtml;
const doIndex = isHTML && opts.indexer && url.indexOf('.plain.html') < 0;

if (isHTML) {
Expand Down Expand Up @@ -322,6 +322,7 @@ window.LiveReloadOptions = {
ctx.log.trace(lines.join('\n'));
}
if (replaceHead) {
await opts.headHtml.setCookie(req.headers.cookie);
textBody = await opts.headHtml.replace(textBody);
}
if (livereload) {
Expand Down
4 changes: 3 additions & 1 deletion src/up.cmd.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,9 @@
const resp = await getFetch()(fstabUrl);
await resp.buffer();
if (!resp.ok) {
if (ref === 'main') {
if (resp.status === 401) {
this.log.warn(chalk`Unable to verify {yellow ${ref}} branch via {blue ${fstabUrl}} for authenticated sites.`);

Check warning on line 123 in src/up.cmd.js

View check run for this annotation

Codecov / codecov/patch

src/up.cmd.js#L123 

Added line #L123 was not covered by tests
} else if (ref === 'main') {
this.log.warn(chalk`Unable to verify {yellow main} branch via {blue ${fstabUrl}} (${resp.status}). Maybe not pushed yet?`);
} else {
this.log.warn(chalk`Unable to verify {yellow ${ref}} branch on {blue ${fstabUrl}} (${resp.status}). Fallback to {yellow main} branch.`);
Expand Down
77 changes: 61 additions & 16 deletions test/head-html.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@ function removePosition(tree) {

async function init(hhs, localHtml, remoteHtml) {
hhs.localHtml = localHtml;
hhs.localStatus = 200;
hhs.remoteHtml = remoteHtml;
hhs.remoteStatus = 200;
if (remoteHtml) {
hhs.remoteDom = await HeadHtmlSupport.toDom(hhs.remoteHtml);
HeadHtmlSupport.hash(hhs.remoteDom);
Expand Down Expand Up @@ -149,7 +151,7 @@ describe('Head.html loading tests', () => {
it('loads remote head.html', async () => {
const directory = await setupProject(path.join(__rootdir, 'test', 'fixtures', 'project'), testRoot);

const scope = nock('https://main--blog--adobe.hlx.page')
nock('https://main--blog--adobe.hlx.page')
.get('/head.html')
.reply(200, '<!-- remote head html --> <script>a=1;</script>');

Expand Down Expand Up @@ -180,13 +182,12 @@ describe('Head.html loading tests', () => {
type: 'root',
});
assert.strictEqual(hhs.remoteStatus, 200);
scope.done();
});

it('loads missing remote head.html', async () => {
const directory = await setupProject(path.join(__rootdir, 'test', 'fixtures', 'project'), testRoot);

const scope = nock('https://main--blog--adobe.hlx.page')
nock('https://main--blog--adobe.hlx.page')
.get('/head.html')
.reply(404);

Expand All @@ -198,10 +199,9 @@ describe('Head.html loading tests', () => {
await hhs.loadRemote();
assert.strictEqual(hhs.remoteDom, null);
assert.strictEqual(hhs.remoteStatus, 404);
scope.done();
});

it('init loads local and remote head.html', async () => {
it('update loads local and remote head.html', async () => {
const directory = await setupProject(path.join(__rootdir, 'test', 'fixtures', 'project'), testRoot);

nock('https://main--blog--adobe.hlx.page')
Expand All @@ -213,20 +213,58 @@ describe('Head.html loading tests', () => {
proxyUrl: 'https://main--blog--adobe.hlx.page',
directory,
});
await hhs.init();
await hhs.update();
assert.strictEqual(hhs.remoteHtml, '<!-- remote head html --><a>fooo</a>');
assert.strictEqual(hhs.remoteStatus, 200);
assert.strictEqual(hhs.localHtml, '<!-- local head html -->\n<link rel="stylesheet" href="/styles.css"/>');
assert.strictEqual(hhs.localStatus, 200);
assert.strictEqual(hhs.isModified, true);

await hhs.init(); // only once
// only once
await hhs.update();
});

it('update loads remote head.html with cookie', async () => {
const directory = await setupProject(path.join(__rootdir, 'test', 'fixtures', 'project'), testRoot);

nock('https://main--blog--adobe.hlx.page')
.get('/head.html')
.reply(function request() {
assert.strictEqual(this.req.headers.cookie, 'foobar');
return [200, '<!-- remote head html --><a>fooo</a>\n'];
});

const hhs = new HeadHtmlSupport({
log: console,
proxyUrl: 'https://main--blog--adobe.hlx.page',
directory,
});
hhs.localStatus = 200;
hhs.setCookie('foobar');
await hhs.update();
});

it('init loads local and remote head.html (not modified)', async () => {
it('update loads remote head.html can handle errors', async () => {
const directory = await setupProject(path.join(__rootdir, 'test', 'fixtures', 'project'), testRoot);

const scope = nock('https://main--blog--adobe.hlx.page')
nock('https://main--blog--adobe.hlx.page')
.get('/head.html')
.reply(404);

const hhs = new HeadHtmlSupport({
log: console,
proxyUrl: 'https://main--blog--adobe.hlx.page',
directory,
});
hhs.localStatus = 200;
await hhs.update();
assert.strictEqual(hhs.remoteStatus, 404);
});

it('update loads local and remote head.html (not modified)', async () => {
const directory = await setupProject(path.join(__rootdir, 'test', 'fixtures', 'project'), testRoot);

nock('https://main--blog--adobe.hlx.page')
.get('/head.html')
.reply(200, '<!-- local head html -->\n<link rel="stylesheet" href="/styles.css"/>');

Expand All @@ -235,34 +273,41 @@ describe('Head.html loading tests', () => {
proxyUrl: 'https://main--blog--adobe.hlx.page',
directory,
});
await hhs.init();
await hhs.update();
assert.strictEqual(hhs.remoteHtml, '<!-- local head html -->\n<link rel="stylesheet" href="/styles.css"/>');
assert.strictEqual(hhs.remoteStatus, 200);
assert.strictEqual(hhs.localHtml, '<!-- local head html -->\n<link rel="stylesheet" href="/styles.css"/>');
assert.strictEqual(hhs.localStatus, 200);
assert.strictEqual(hhs.isModified, false);

await hhs.init(); // only once
scope.done();
// only once
await hhs.update();
});

it('init sets modified to false if local status failed', async () => {
it('update sets modified to false if local status failed', async () => {
const hhs = new HeadHtmlSupport(DEFAULT_OPTS());
hhs.localHtml = '';
hhs.remoteHtml = '';
hhs.localStatus = 404;
hhs.remoteStatus = 200;
await hhs.init();
await hhs.update();
assert.strictEqual(hhs.isModified, false);
});

it('init sets modified to false if remote status failed', async () => {
it('invalidate local invalidates the status', async () => {
const hhs = new HeadHtmlSupport(DEFAULT_OPTS());
hhs.localStatus = 404;
hhs.invalidateLocal();
assert.strictEqual(hhs.localStatus, 0);
});

it('update sets modified to false if remote status failed', async () => {
const hhs = new HeadHtmlSupport(DEFAULT_OPTS());
hhs.localHtml = '';
hhs.remoteHtml = '';
hhs.localStatus = 200;
hhs.remoteStatus = 404;
await hhs.init();
await hhs.update();
assert.strictEqual(hhs.isModified, false);
});
});
24 changes: 3 additions & 21 deletions test/server.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,10 +104,6 @@ describe('Helix Server', () => {
.withProxyUrl('https://main--foo--bar.hlx.page/')
.withHttpPort(0);

nock('https://main--foo--bar.hlx.page')
.get('/head.html')
.reply(200, '<link rel="stylesheet" href="/styles.css"/>');

await project.init();
try {
await project.start();
Expand All @@ -124,10 +120,6 @@ describe('Helix Server', () => {
.withProxyUrl('https://main--foo--bar.hlx.page/')
.withHttpPort(0);

nock('https://main--foo--bar.hlx.page')
.get('/head.html')
.reply(200, '<link rel="stylesheet" href="/styles.css"/>');

await project.init();
try {
await project.start();
Expand Down Expand Up @@ -164,9 +156,7 @@ describe('Helix Server', () => {

nock('http://main--foo--bar.hlx.page')
.get('/notfound.css')
.reply(404)
.get('/head.html')
.reply(200, '<link rel="stylesheet" href="/styles.css"/>');
.reply(404);

try {
await project.start();
Expand All @@ -188,9 +178,7 @@ describe('Helix Server', () => {
nock('http://main--foo--bar.hlx.page')
.get('/local.html')
.optionally(true)
.reply(200, 'foo')
.get('/head.html')
.reply(200, '<link rel="stylesheet" href="/styles.css"/>');
.reply(200, 'foo');

try {
await project.start();
Expand Down Expand Up @@ -219,9 +207,7 @@ describe('Helix Server', () => {
.get('/missing')
.reply(404, 'server 404 html', {
'content-type': 'text/html',
})
.get('/head.html')
.reply(200, '<link rel="stylesheet" href="/styles.css"/>');
});

try {
await project.start();
Expand Down Expand Up @@ -366,10 +352,6 @@ describe('Helix Server', () => {
// a request like: /subfolder/query-index.json?sheet=foo&limit=20?sheet=foo&limit=20
assert.strictEqual(uri, '/subfolder/query-index.json?sheet=foo&limit=20');
return [200, '{ "data": [] }', { 'content-type': 'application/json' }];
})
.get('/head.html')
.reply(200, '<link rel="stylesheet" href="/styles.css"/>', {
'content-type': 'text/html',
});

try {
Expand Down
24 changes: 6 additions & 18 deletions test/up-cmd.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,7 @@ describe('Integration test for up command with helix pages', function suite() {
.get('/index.html')
.reply(200, '## Welcome')
.get('/not-found.txt')
.reply(404)
.get('/head.html')
.reply(200, '<link rel="stylesheet" href="/styles.css"/>');
.reply(404);

let port;
await new Promise((resolve, reject) => {
Expand Down Expand Up @@ -116,9 +114,7 @@ describe('Integration test for up command with helix pages', function suite() {
.get('/index.html')
.reply(200, '## Welcome')
.get('/not-found.txt')
.reply(404)
.get('/head.html')
.reply(200, '<link rel="stylesheet" href="/styles.css"/>');
.reply(404);

cmd
.on('started', async () => {
Expand Down Expand Up @@ -158,9 +154,7 @@ describe('Integration test for up command with helix pages', function suite() {
.get('/index.html')
.reply(200, '## Welcome')
.get('/not-found.txt')
.reply(404)
.get('/head.html')
.reply(200, '<link rel="stylesheet" href="/styles.css"/>');
.reply(404);

nock('https://master--dummy-foo--adobe.hlx.page')
.get('/fstab.yaml')
Expand Down Expand Up @@ -204,19 +198,15 @@ describe('Integration test for up command with helix pages', function suite() {
.get('/index.html')
.reply(200, '## Welcome')
.get('/not-found.txt')
.reply(404)
.get('/head.html')
.reply(200, '<link rel="stylesheet" href="/styles.css"/>');
.reply(404);

nock('https://master--dummy-foo--adobe.hlx.page')
.get('/fstab.yaml')
.reply(404, 'dummy');

nock('https://new-branch--dummy-foo--adobe.hlx.page')
.get('/fstab.yaml')
.reply(200, 'yep!')
.get('/head.html')
.reply(200, '## Welcome');
.reply(200, 'yep!');

let timer;
cmd
Expand Down Expand Up @@ -261,9 +251,7 @@ describe('Integration test for up command with helix pages', function suite() {
.get('/index.html')
.reply(200, '## Welcome')
.get('/not-found.txt')
.reply(404)
.get('/head.html')
.reply(200, '<link rel="stylesheet" href="/styles.css"/>');
.reply(404);

nock('https://master--dummy-foo--adobe.hlx.page')
.get('/fstab.yaml')
Expand Down
Loading