Feat/configinlink #1146

brianlmacdonald · 2018-02-28T16:46:07Z

- Summary

Addressed issue #1132. The cms configuration file's URL had been hardcoded before. This PR allows the user to provide a <link> in <head> to direct to a config file elsewhere. We grab the link out of head using a query looking for rel="cms-config-url", then check to make sure the type is valid and supported. If the query finds a link and it's valid we grab the href provided, otherwise we use the default config.yml url.

- Test plan

Aside from creating tests, I ran the CMS example code with and without the <link> . Without the url defaults to config.yml and it works. To test the <link> url, I created a new folder and moved the config.yml file there and had the href pointing to it. It worked. I also added <link> s with incorrect type and it threw the expected error.

- Description for the changelog

Allows setting config URL with <link> in <head> .

- A picture of a cute animal (not mandatory but encouraged)
Behold, a slow loris!

verythorough · 2018-02-28T16:48:22Z

Deploy preview for cms-demo ready!

Built with commit 6348f0c

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

verythorough · 2018-02-28T16:48:32Z

Deploy preview for netlify-cms-www ready!

Built with commit 6348f0c

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

erquhart

Thanks for this!

So the use case being covered is somewhat advanced, and also very simple. Therefore, I'm thinking:

No need to provide error messaging.
No need to export just for testing purposes.
We can slim this whole thing down to something like:

configUrl = _.get(document.querySelector('link[rel="netlify-cms-config"]'), 'href') || 'config.yml'

Expanding the query a bit to ensure the type is set is fine too, but that's about it.

tech4him1 · 2018-03-02T21:22:49Z

~~@erquhart If we want to eventually expand this to support multiple formats, I'm not sure we want to "slim it down" too much -- we still want to check that the format is correct, for example. Thoughts?~~

brianlmacdonald · 2018-03-02T21:26:20Z

@tech4him1 working on checking the type as we speak.

tech4him1 · 2018-03-02T21:57:29Z

src/actions/config.js

@@ -7,6 +7,12 @@ export const CONFIG_REQUEST = "CONFIG_REQUEST";
 export const CONFIG_SUCCESS = "CONFIG_SUCCESS";
 export const CONFIG_FAILURE = "CONFIG_FAILURE";

+const configUrl =
+  get(
+    get(document.querySelectorAll('head link[rel="cms-config-url"][type="text/yaml"], [type="application/x-yaml"]')[0],


I don't believe head link[rel="cms-config-url"][type="text/yaml"], [type="application/x-yaml"] is valid, since you are checking for either head link[rel="cms-config-url"][type="text/yaml"] or [type="application/x-yaml"].

Two thoughts:

Just use querySelector and take the first one. I don't think we need robust handling here - it's an advanced use case, they should just make sure they don't add more than one.

It's cool to check the type property of the retrieved element directly, a bit cleaner too.

tech4him1 · 2018-03-02T22:07:55Z

I think we should check the format separately from the selector, so that it is easier to add support for multiple formats later. Thoughts?

talves · 2018-03-03T17:39:43Z

Am I being too robust to ask we include support for json here?
I have some plans in the works for a configurator app for the cms config and would like to provide a json endpoint. If it is out of scope, I can always PR a change later.

@Benaiah and I were discussing this the other day and want to make sure he has reviewed this PR also. 😄

tech4him1 · 2018-03-05T18:24:29Z

src/actions/config.js

 import { authenticateUser } from "Actions/auth";
 import * as publishModes from "Constants/publishModes";

 export const CONFIG_REQUEST = "CONFIG_REQUEST";
 export const CONFIG_SUCCESS = "CONFIG_SUCCESS";
 export const CONFIG_FAILURE = "CONFIG_FAILURE";

-const validTypes = [ "text/yaml", "application/x-yaml" ];
+const validTypes = { TEXT_YAML: "text/yaml", APPLICATION_X_YAML: "application/x-yaml" };


Why not just use an array of types?

@tech4him1 I was looking through other files and saw types as an object in ListControl.js , so I figured I'd follow in previous footsteps. I can revert back to an array, if you'd like?

Let me think a little about how this would best work with our formats.js for supporting multiple config formats in the future. (if you have any suggestions, that's great as well)

tech4him1 · 2018-03-15T19:05:01Z

@brianlmacdonald Sorry for the delay, I'm ready to get this merged! I'm thinking the easiest way we would have to send this into our formats.js, is to have the accepted MIME types as the key and the formatter "name" as the value. The keys/values could be switched too, if that is easier.

Example:

{
  "text/yaml": "yaml", 
  "application/x-yaml": "yaml",
  "application/toml": "toml",
}

brianlmacdonald · 2018-03-15T19:55:21Z

@tech4him1 sounds good, I made the adjustments.

erquhart · 2018-03-15T20:50:34Z

src/actions/config.js

@@ -9,6 +9,11 @@ export const CONFIG_SUCCESS = "CONFIG_SUCCESS";
 export const CONFIG_FAILURE = "CONFIG_FAILURE";
 export const CONFIG_MERGE = "CONFIG_MERGE";

+const validTypes = { 'text/yaml': 'yaml', 'application/x-yaml': 'yaml', 'application/toml': 'toml' };


Unless we add toml parsing for the config, we can only handle json or yaml.

erquhart · 2018-03-15T20:50:36Z

src/actions/config.js

+const validTypes = { 'text/yaml': 'yaml', 'application/x-yaml': 'yaml', 'application/toml': 'toml' };
+const configLink = document.querySelector('link[rel="cms-config-url"]');
+const isValidType = link => link && validTypes[link.type]; 
+const configUrl = isValidType(configLink) ? get(configLink, 'href') : 'config.yml';


It'd be nice to group all of the logic into a getConfigUrl function.

You got it!

tech4him1 · 2018-03-16T14:30:56Z

src/actions/__tests__/config.spec.js

+
+  describe('getConfig', () => {
+    const validTypes = { 'text/yaml': 'yaml', 'application/x-yaml': 'yaml', 'application/toml': 'toml' };


Looks like TOML got left in the test accidently.

tech4him1 · 2018-03-16T15:18:23Z

We may need to add docs at some point as well, but that can probably wait until we get more real-world use cases.

brianlmacdonald · 2018-03-16T15:21:44Z

@tech4him1 I can help with docs as soon as I finish battling the field vs fields bug.

tech4him1 · 2018-03-16T15:21:48Z

src/actions/config.js

@@ -107,8 +114,9 @@ export function loadConfig() {
    dispatch(configLoading());

    try {
+      const configUrl = getConfig();


If you don't mind, I think it would be good to add something like:

console.log(`Netlify CMS using config file: "${ configURL }"`);

That way if a config file isn't being recognized and is defaulting to config.yml, at least the dev knows that is happening.

tech4him1 · 2018-03-16T15:23:15Z

Awesome, let's get it merged! Really appreciate all your work on this!

tech4him1 · 2018-03-16T15:29:20Z

Am I being too robust to ask we include support for json here?

@talves For now, our YAML parser works with JSON 😄:

<link rel="cms-config-url" type="text/yaml" href="config.json">

erquhart

Couple more things here, one may cause the CMS to crash. We should also add some docs to this before merging.

erquhart · 2018-03-28T19:44:23Z

src/actions/config.js

+const getConfig = () => {
+  const validTypes = { 'text/yaml': 'yaml', 'application/x-yaml': 'yaml' };
+  const configLink = document.querySelector('link[rel="cms-config-url"]');
+  const isValidType = link => link && validTypes[link.type];


Looks like we're going to crash if there's no matching link element by attempting to access property "type" of null.

@erquhart doesn't link && validTypes[link.type] short circuit to false when link is null?

🤦‍♂️🤦‍♂️🤦‍♂️

Disregard!

erquhart · 2018-03-28T19:47:56Z

example/index.html

@@ -7,6 +7,9 @@

  <link href='https://fonts.googleapis.com/css?family=Roboto:300,400,500' rel='stylesheet' type='text/css'>
  <link rel="stylesheet" href="/cms.css"/>
+<link href="config.yml" type="text/yaml" rel="cms-config-url">


We should remove this from the example project, only because it makes an optional feature seem necessary, and a lot of people treat this example as documentation. We could, however add a comment like:

tech4him1 · 2018-03-28T20:55:41Z

@brianlmacdonald Would you mind rebasing onto master? Just makes the diff a bit nicer to review 😄.

brianlmacdonald · 2018-03-28T21:02:20Z

@tech4him1 -- I know. It looks horrible. Lesson learned.

erquhart · 2018-03-29T18:19:38Z

@talves if you have a sec can you test this against one of your local cases? Just want to be sure before merging.

tech4him1 · 2018-04-10T00:31:47Z

@talves Did you get a chance to test this yet?

talves · 2018-04-10T01:04:13Z

@tech4him1 not yet, sorry. I will try to see if I can get to it tomorrow, if you need it soon.

tech4him1 · 2018-04-10T01:16:10Z

@talves no rush on my part, just wanted to be sure you knew about it 😄

talves

We need to fix the default failure on a bad link path at a minimum. This is a great contribution btw!

talves · 2018-04-10T17:11:23Z

src/actions/config.js

@@ -130,7 +137,9 @@ export function loadConfig() {

    try {
      const preloadedConfig = getState().config;
-      const loadedConfig = await getConfig('config.yml', preloadedConfig && preloadedConfig.size > 1);
+      const configUrl = getConfigUrl();
+      console.log(`Netlify CMS using config file: "${configUrl}"`);


I wonder if we should clarify this a little further. Also, maybe only show it if the link exists since the default behavior is already known. So, this console.log would move into the getConfigUrl function. That way when debugging we know the link was used rather than default. Not detrimental though, just a thought.

Using config file path: "${configUrl}"

Probably don't need to specify Netilify CMS since we already print a version on page load showing Netlify CMS

@talves Awesome! Made the changes!

talves · 2018-04-10T17:31:04Z

src/actions/config.js

+  const validTypes = { 'text/yaml': 'yaml', 'application/x-yaml': 'yaml' };
+  const configLink = document.querySelector('link[rel="cms-config-url"]');
+  const isValidType = link => link && validTypes[link.type];
+  return isValidType(configLink) ? get(configLink, 'href') : 'config.yml';


This returns an empty path for the default if the user supplies a link without an href. It really should return 'config.yml' on any invalid form of the link.

Proposed:

const getConfigUrl = () => { const validTypes = { 'text/yaml': 'yaml', 'application/x-yaml': 'yaml' }; const configLinkEl = document.querySelector('link[rel="cms-config-url"]'); const isValidLink = configLinkEl && validTypes[configLinkEl.type] && get(configLinkEl, 'href'); if (isValidLink) { console.log(`Using config file path: "${configUrl}"`); return get(configLinkEl, 'href'); } return 'config.yml'; }

contributor addition moved to decaporg#1241

talves · 2018-04-10T18:23:17Z

src/actions/config.js

+  const configLinkEl = document.querySelector('link[rel="cms-config-url"]');
+  const isValidLink = configLinkEl && validTypes[configLinkEl.type] && get(configLinkEl, 'href');
+  if (isValidLink) {
+    console.log(`Using config file path: "${configUrl}"`);


Sorry Brian, missed the configUrl value here. I will let you decide how to manage it 😄

With it fixed, things look good after that.

@talves done and done!

talves

All Looks good. Testing well in my use cases also!

Benaiah · 2018-04-10T20:43:19Z

Going to squash and merge this as soon as the Travis check completes. @brianlmacdonald thanks for the PR!

* fix: allows for valid config types expansion * feat: config url can now come through link tag * fix: lints added coded * fix: slims down code per review * fix: expands query to find supported type * fix: removes typo in test copy * fix: changes validTypes to object * fix: groups config functions into one getConfig func * adds console message for config url * adds to docs * update docs * fix test * fix merge conflicts contributor addition moved to decaporg#1241 * avoids empty path with link without href. changes link console message * removes additional console * fixes link path in console * fix: remove superfluous .allcontributorsrc change

erquhart suggested changes Mar 2, 2018

View reviewed changes

tech4him1 reviewed Mar 2, 2018

View reviewed changes

talves mentioned this pull request Mar 3, 2018

Allow setting config URL with link element in head #1132

Closed

tech4him1 reviewed Mar 5, 2018

View reviewed changes

erquhart suggested changes Mar 15, 2018

View reviewed changes

tech4him1 reviewed Mar 16, 2018

View reviewed changes

tech4him1 approved these changes Mar 16, 2018

View reviewed changes

tech4him1 reviewed Mar 16, 2018

View reviewed changes

tech4him1 requested a review from erquhart March 16, 2018 15:22

tech4him1 added area: api impact: docs impact: changelog area: configuration impact: configuration labels Mar 16, 2018

erquhart suggested changes Mar 28, 2018

View reviewed changes

erquhart approved these changes Mar 29, 2018

View reviewed changes

brianlmacdonald and others added 12 commits April 9, 2018 18:29

fix: allows for valid config types expansion

4979c64

feat: config url can now come through link tag

05348f6

fix: lints added coded

7d9906b

fix: slims down code per review

adcc171

fix: expands query to find supported type

6ec83cd

fix: removes typo in test copy

e44c79f

fix: changes validTypes to object

d118c54

fix: groups config functions into one getConfig func

5c1cfb3

adds console message for config url

68518fa

adds to docs

a888f39

update docs

9d4a8a1

fix test

681f98d

tech4him1 force-pushed the feat/configinlink branch from 5194660 to 681f98d Compare April 10, 2018 00:31

talves requested changes Apr 10, 2018

View reviewed changes

tech4him1 and others added 3 commits April 10, 2018 14:02

fix merge conflicts

4d2d3a9

contributor addition moved to decaporg#1241

avoids empty path with link without href. changes link console message

3d7bd0b

removes additional console

c75061d

talves reviewed Apr 10, 2018

View reviewed changes

fixes link path in console

c738a82

talves approved these changes Apr 10, 2018

View reviewed changes

Benaiah approved these changes Apr 10, 2018

View reviewed changes

fix: remove superfluous .allcontributorsrc change

6348f0c

Benaiah merged commit 6b77aee into decaporg:master Apr 10, 2018


		describe('getConfig', () => {
		const validTypes = { 'text/yaml': 'yaml', 'application/x-yaml': 'yaml', 'application/toml': 'toml' };

Feat/configinlink #1146

Feat/configinlink #1146

Conversation

brianlmacdonald commented Feb 28, 2018 • edited Loading

verythorough commented Feb 28, 2018 • edited Loading

verythorough commented Feb 28, 2018 • edited Loading

erquhart left a comment • edited Loading

Choose a reason for hiding this comment

tech4him1 commented Mar 2, 2018 • edited Loading

brianlmacdonald commented Mar 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tech4him1 commented Mar 2, 2018 • edited Loading

talves commented Mar 3, 2018

Choose a reason for hiding this comment

brianlmacdonald Mar 5, 2018 • edited Loading

Choose a reason for hiding this comment

tech4him1 Mar 5, 2018 • edited Loading

Choose a reason for hiding this comment

tech4him1 commented Mar 15, 2018 • edited Loading

brianlmacdonald commented Mar 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tech4him1 commented Mar 16, 2018 • edited Loading

brianlmacdonald commented Mar 16, 2018

tech4him1 Mar 16, 2018 • edited Loading

Choose a reason for hiding this comment

tech4him1 commented Mar 16, 2018

tech4him1 commented Mar 16, 2018

erquhart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tech4him1 commented Mar 28, 2018 • edited Loading

brianlmacdonald commented Mar 28, 2018

erquhart commented Mar 29, 2018

tech4him1 commented Apr 10, 2018

talves commented Apr 10, 2018

tech4him1 commented Apr 10, 2018 • edited Loading

talves left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talves Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

talves Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talves left a comment

Choose a reason for hiding this comment

Benaiah commented Apr 10, 2018 • edited Loading

brianlmacdonald commented Feb 28, 2018 •

edited

Loading

verythorough commented Feb 28, 2018 •

edited

Loading

verythorough commented Feb 28, 2018 •

edited

Loading

erquhart left a comment •

edited

Loading

tech4him1 commented Mar 2, 2018 •

edited

Loading

tech4him1 commented Mar 2, 2018 •

edited

Loading

brianlmacdonald Mar 5, 2018 •

edited

Loading

tech4him1 Mar 5, 2018 •

edited

Loading

tech4him1 commented Mar 15, 2018 •

edited

Loading

tech4him1 commented Mar 16, 2018 •

edited

Loading

tech4him1 Mar 16, 2018 •

edited

Loading

tech4him1 commented Mar 28, 2018 •

edited

Loading

tech4him1 commented Apr 10, 2018 •

edited

Loading

talves Apr 10, 2018 •

edited

Loading

talves Apr 10, 2018 •

edited

Loading

Benaiah commented Apr 10, 2018 •

edited

Loading