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.

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

Allow for un-captured route of config.yml #1182

Merged
merged 4 commits into from
Mar 28, 2018
Merged

Conversation

talves
Copy link
Collaborator

@talves talves commented Mar 19, 2018

This fixes if there was an invalid route to the config.yml because it did not exist and it was handled without a 404.

Delete this branch when merged.

@verythorough
Copy link
Contributor

verythorough commented Mar 19, 2018

Deploy preview for netlify-cms-www ready!

Built with commit 82e450c

https://deploy-preview-1182--netlify-cms-www.netlify.com

@verythorough
Copy link
Contributor

verythorough commented Mar 19, 2018

Deploy preview for cms-demo ready!

Built with commit 82e450c

https://deploy-preview-1182--cms-demo.netlify.com

@talves talves requested a review from erquhart March 19, 2018 23:35
@talves
Copy link
Collaborator Author

talves commented Mar 19, 2018

@erquhart This works, but was not sure if I needed to actually parse the empty string config:
let loadedConfig = parseConfig('');

@talves talves changed the title Allow for un-captured route (WIP) Allow for un-captured route of config.yml Mar 26, 2018
if (!preloadedConfig && !requestSuccess) {
throw new Error(`Failed to load config.yml (${ response.status })`);
if (!preloadedConfig && !fetchedConfig) {
throw new Error(`Failed to load configuration.`);
Copy link
Contributor

Choose a reason for hiding this comment

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

The change in error message is to open up to the config no longer being "config.yml" with the understanding that a failed request will already generate a message in the console.

@erquhart
Copy link
Contributor

erquhart commented Mar 27, 2018

Added a commit for that, can you test against your setup?

@talves
Copy link
Collaborator Author

talves commented Mar 27, 2018

The latest commit was what I was proposing to combine the checking of the validity of a yaml download and returning a config based on whether there was a valid file existing and if the preloaded configuration actually exists.

This would also allow for a reload of a configuration later using a different action in the case we end up using dynamic configuration loading. It will also allow for dynamic file path locations to the yaml.

isPreloaded defines whether we are going to allow for the config to exist or not, so this should not be bound outside of the getting the config only deciding whether we allow and empty config to return on failure to exist. The validation of the config on merge will determine whether a valid config was loaded.

throw new Error(`Failed to load config.yml (${ response.status })`);
}
const contentType = response.headers.get('Content-Type') || 'Not-Found';
const isYaml = contentType.indexOf('yaml') !== -1;
Copy link
Collaborator Author

@talves talves Mar 27, 2018

Choose a reason for hiding this comment

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

There are different type strings for yaml that can exist, so this should allow for each. After some thought, I think we can allow for an external location later as long as it returns a valid header type for yaml, it should be accepted. Later we could allow for a preloaded path in the preloaded config that could supply the location of the file.

}

const loadedConfig = parseConfig(requestSuccess ? await response.text() : '');
const loadedConfig = await getConfig('config.yml', preloadedConfig && preloadedConfig.size > 1);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In testing, preloadedConfig has an iterable object map with one key isFetching if the preload did not have a JSON preload using init().

@talves talves changed the title (WIP) Allow for un-captured route of config.yml Allow for un-captured route of config.yml Mar 27, 2018
@talves
Copy link
Collaborator Author

talves commented Mar 28, 2018

@erquhart did you forget to merge this prior to #1173 ?

@erquhart erquhart changed the base branch from init-once to master March 28, 2018 20:03
@erquhart erquhart force-pushed the talves-patch-init-once branch from a4f9b08 to dcb3676 Compare March 28, 2018 20:17
@erquhart erquhart force-pushed the talves-patch-init-once branch from dcb3676 to 82e450c Compare March 28, 2018 20:18
@erquhart erquhart merged commit d5cd79f into master Mar 28, 2018
@erquhart erquhart deleted the talves-patch-init-once branch March 28, 2018 20:25
brianlmacdonald pushed a commit to brianlmacdonald/netlify-cms that referenced this pull request May 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants