Commit 0de918ff authored by Kyle Keating's avatar Kyle Keating Committed by Kyle Keating

Remove options.defaultValue from hostPageSetting

- defaultValue is now handled inside of getConfig so this code is unused.

- hostPageSetting now returns undefined, rather than null, if a value is not found in the config. The undefined value will trigger the defaultValue to take effect in getConfig if a defaultValue is provided.
parent da3a3466
......@@ -84,7 +84,7 @@ export default function settingsFrom(window_) {
function showHighlights() {
let showHighlights_ = hostPageSetting('showHighlights');
if (showHighlights_ === null) {
if (showHighlights_ === undefined) {
showHighlights_ = 'always'; // The default value is 'always'.
}
......@@ -128,9 +128,17 @@ export default function settingsFrom(window_) {
return jsonConfigs.query || queryFromURL();
}
function hostPageSetting(name, options = {}) {
const hasDefaultValue = typeof options.defaultValue !== 'undefined';
/**
* Returns the first setting value found from the respective sources in order.
*
* 1. window.hypothesisConfig()
* 2. <script class="js-hypothesis-config">
*
* If the setting is not found in either source, then return undefined.
*
* @param {string} name - Unique name of the setting
*/
function hostPageSetting(name) {
if (configFuncSettings.hasOwnProperty(name)) {
return configFuncSettings[name];
}
......@@ -139,11 +147,7 @@ export default function settingsFrom(window_) {
return jsonConfigs[name];
}
if (hasDefaultValue) {
return options.defaultValue;
}
return null;
return undefined;
}
return {
......
......@@ -298,18 +298,15 @@ describe('annotator/config/settingsFrom', () => {
input: 42,
output: 42,
},
// If the host page sets showHighlights to null this will be mistaken
// for the host page not containing a showHighlights setting at all and
// showHighlights will be set to 'always'.
{
it: 'defaults to "always"',
input: null,
input: undefined,
output: 'always',
},
{
it: 'passes undefined through unmodified',
input: undefined,
output: undefined,
it: 'passes null through unmodified',
input: null,
output: null,
},
{
it: 'passes arrays through unmodified',
......@@ -367,7 +364,7 @@ describe('annotator/config/settingsFrom', () => {
isBrowserExtension: false,
configFuncSettings: { ignoreOtherConfiguration: '1' },
jsonSettings: { foo: 'ignored' },
expected: null,
expected: undefined,
},
{
when: 'the client is embedded in a web page',
......@@ -400,41 +397,13 @@ describe('annotator/config/settingsFrom', () => {
jsonSettings: { foo: 'jsonValue' },
expected: undefined,
},
{
when: 'no default value is provided',
specify: 'it returns null',
configFuncSettings: {},
jsonSettings: {},
defaultValue: undefined,
expected: null,
},
{
when: 'a default value is provided',
specify: 'it returns that default value',
configFuncSettings: {},
jsonSettings: {},
defaultValue: 'test value',
expected: 'test value',
},
{
when: 'a default value is provided but it is overridden',
specify: 'it returns the overridden value',
configFuncSettings: { foo: 'not the default value' },
jsonSettings: {},
defaultValue: 'the default value',
expected: 'not the default value',
},
].forEach(function (test) {
context(test.when, () => {
specify(test.specify, () => {
fakeConfigFuncSettingsFrom.returns(test.configFuncSettings);
fakeParseJsonConfig.returns(test.jsonSettings);
const settings = settingsFrom(fakeWindow());
const setting = settings.hostPageSetting('foo', {
defaultValue: test.defaultValue || null,
});
const setting = settings.hostPageSetting('foo');
assert.strictEqual(setting, test.expected);
});
});
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment