Skip load `config.yml` config #1866

LoicMahieu · 2018-11-09T14:02:51Z

Summary

I would like to make config more "programmable". Collections may comes from functions, uses reusable fields etc...

With this PR, we'll be able to specify config only by JS with a manual init.

verythorough · 2018-11-09T14:09:22Z

Deploy preview for netlify-cms-www ready!

Built with commit 59340ad

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

talves · 2018-11-09T16:12:01Z

Can you explain with an example of what it changes? Not sure I am following how your change improves the current process.

Specifically, I want to make sure we are not breaking current functionality. Also want to see what it adds also.

LoicMahieu · 2018-11-09T16:25:22Z

@talves Sure! Sorry if it is not clear. Don't hesitate to take me back if it's still not.

My ambition is the declare the whole config in JS. Since whole config is loaded at init time, this is not necessary to load an empty config.yml.
Also it'll allow to not depends on this static file if the environment does not permit it.

Another thing is that configuration will be centralized in one file. If you use a "dynamic" backend configuration, for example, switch to different branch from environment variable, you will have a project like:

.
├── public
│   └── admin
│       └── config.yml
└── src
    └── cms
        └── cms.js

Configuration is split in 2 folders located at the opposite side of the project.

I want to use JS only for configuration because my collections are very similar and I would like to generate them from utilities functions:

const createFooCollection = (options) => ({ name: options.name, fields: ... })

verythorough · 2018-11-09T16:49:12Z

Deploy preview for cms-demo ready!

Built with commit 59340ad

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

packages/netlify-cms-core/src/actions/config.js

erquhart · 2018-11-13T23:58:08Z

packages/netlify-cms-core/src/actions/config.js

@@ -111,12 +106,15 @@ export function loadConfig() {
    try {
      const preloadedConfig = getState().config;


I don't believe this has a role any longer with the changes in this PR.

Surely, I confess that I can't say if it is still relevant. Do you confirm that we can remove this ?

Yeah, passing bootstrapConfig into loadConfig supersedes the old preloadedConfig behavior, you can drop it.

erquhart · 2018-11-22T01:16:33Z

@talves would you have time to give this a quick review?

talves · 2018-11-22T18:37:01Z

@erquhart isn't this still waiting for your requested changes?

erquhart · 2018-11-22T18:42:18Z

He added commits to resolve those.

_{Sent with GitHawk}

talves · 2018-11-22T18:49:39Z

#1866 (comment) has been resolved?

Sorry, I am not seeing a commit for it.

erquhart · 2018-11-22T19:05:06Z

Ah, that one is just eliminating dead code, no actual impact on behavior.

_{Sent with GitHawk}

talves · 2018-11-24T19:26:43Z

I just can't get my head around why we want to pass the bootstrapConfig object to the App component as a prop to make this change. We can already load a config using the import of init/bootstrap.

import React, { Component } from 'react';
import CMS, { init } from 'netlify-cms';

const config = {
  "backend": {
    "name": "test-repo"
  },
  "display_url": "https://example.com",
  "publish_mode": "editorial_workflow",
  "media_folder": "assets/uploads",
  "load_config_file": false,
  "collections": [
    {
      "name": "posts",
      "label": "Posts",
      "label_singular": "Post",
      "description": "The description is a great place for tone setting, high level information, and editing guidelines that are specific to a collection.\n",
      "folder": "_posts",
      "slug": "{{year}}-{{month}}-{{day}}-{{slug}}",
      "create": true,
      "fields": [
        {
          "label": "Title",
          "name": "title",
          "widget": "string",
          "tagname": "h1"
        },
        {
          "label": "Publish Date",
          "name": "date",
          "widget": "datetime",
          "dateFormat": "YYYY-MM-DD",
          "timeFormat": "HH:mm",
          "format": "YYYY-MM-DD HH:mm"
        }
      ]
    }
  ]
};

CMS.init = init;

class NetlifyCMS extends Component {
  componentDidMount () {
    CMS.init({config});
  }
  render() {
    return (
      <div id="nc-root" />
    );
  }
}

export default NetlifyCMS;

I do like the idea of ignoring the config.yml based on a flag in the init config, because we can avoid the error of a missing config.yml and request. I think we can do it with just the simple check proposed. The only change really needed is:

src/actions/config.js line 114 to:

const loadedConfig =
        preloadedConfig && preloadedConfig.load_config_file === false
          ? {}
          : await getConfig(configUrl, preloadedConfig && preloadedConfig.size > 1);

erquhart · 2018-11-26T21:50:50Z

Really good point @talves, didn't even notice that. @LoicMahieu any thoughts on Tony's comment? We should remove any unnecessary changes to reduce risk.

LoicMahieu · 2018-12-05T16:40:06Z

I just can't get my head around why we want to pass the bootstrapConfig object to the App component as a prop to make this change. We can already load a config using the import of init/bootstrap.

Indeed...

Really good point @talves, didn't even notice that. @LoicMahieu any thoughts on Tony's comment? We should remove any unnecessary changes to reduce risk.

I confess that I am lacking of time to refactor this changes in a new branch with only these "small" changes.
Furthermore, I don't think theses changes introduce a breaking change (but certainly should be released as minor). I agree that we should minimize the risk to break something but something, it is necessary to go ahead.

…-config * official/master: (48 commits) docs: fix broken link to dev configuration file (decaporg#1941) docs: link releases to tag comparisons update release ticker Publish feat: add cloudinary support (decaporg#1932) fix(netlify-cms-core): duplicate key warning (decaporg#1930) Update image.md (decaporg#1923) chore(netlify-cms-core): upgrade react-frame-component to 4.x (decaporg#1925) chore(netlify-cms-core): upgrade gray-matter to 4.x (decaporg#1924) feat(netlify-cms-widget-select): add support for multiple selection (decaporg#1901) chore: improve publish scripts Publish chore: test before publishing chore(netlify-cms-core): upgrade react-dnd to 7.x (decaporg#1922) chore(netlify-cms-widget-text): upgrade to react-textarea-autosize 7.x (decaporg#1921) chore(netlify-cms-core): upgrade to react-waypoint 8.x (decaporg#1920) fix(backend): use singular label in custom commit message (decaporg#1917) docs: add GAE-specific oAuth client (decaporg#1918) fix(netlify-cms-widget-text): set correct font family (decaporg#1916) chore(netlify-cms-core): upgrade redux and related dependencies (decaporg#1914) ...

erquhart

@talves after revisiting, this does seem a worthwhile improvement, especially considering our Netlify-CMS-as-a-component ambitions. It does include a breaking change, so considering just dropping as abandoned for now and revisiting in the future. What do you think?

erquhart · 2019-02-02T21:24:27Z

packages/netlify-cms-core/src/actions/config.js


      /**
       * Merge any existing configuration so the result can be validated.
       */
-      const mergedConfig = mergePreloadedConfig(preloadedConfig, loadedConfig);
+      const mergedConfig = mergePreloadedConfig(bootstrapConfig, loadedConfig, window.CMS_CONFIG);


This is breaking, window.CMS_CONFIG currently overrides any other config loading if present.

talves · 2019-02-02T21:58:15Z

I agree.

Do you want me to send a PR to only do what I said in this comment, since it would not be a breaking change?

erquhart · 2019-02-02T22:27:58Z

That'd be awesome 👍

erquhart · 2019-02-05T21:56:15Z

Closing in favor of #2053.

LoicMahieu force-pushed the feature/bootstrap-config branch 2 times, most recently from accc5ed to 39acd47 Compare November 9, 2018 16:04

LoicMahieu force-pushed the feature/bootstrap-config branch from 39acd47 to bc05701 Compare November 9, 2018 16:40

talves reviewed Nov 9, 2018

View reviewed changes

packages/netlify-cms-core/src/actions/config.js Outdated Show resolved Hide resolved

Bootstrap config and avoid fetch config.yml

bea25af

LoicMahieu force-pushed the feature/bootstrap-config branch from bc05701 to bea25af Compare November 10, 2018 23:23

erquhart suggested changes Nov 13, 2018

View reviewed changes

LoicMahieu added 2 commits November 21, 2018 15:38

config: restore mergeDeep

b36f097

config: rename load_config_file

51d4892

erquhart mentioned this pull request Nov 26, 2018

config.yml could not be found with manual init #1908

Closed

Remove preloadedConfig, simplify config load

09ad3c3

LoicMahieu force-pushed the feature/bootstrap-config branch from cf3ace6 to 09ad3c3 Compare December 5, 2018 16:39

LoicMahieu mentioned this pull request Dec 5, 2018

feat(editor): add field option when to be able to show fields conditionally #1894

Closed

LoicMahieu added 2 commits December 7, 2018 09:57

Fix lint

8f700e0

erquhart suggested changes Feb 2, 2019

View reviewed changes

talves mentioned this pull request Feb 3, 2019

Skip the config.yml file on init() #2053

Merged

erquhart closed this Feb 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip load `config.yml` config #1866

Skip load `config.yml` config #1866

LoicMahieu commented Nov 9, 2018

verythorough commented Nov 9, 2018 •

edited

Loading

talves commented Nov 9, 2018 •

edited

Loading

LoicMahieu commented Nov 9, 2018

verythorough commented Nov 9, 2018 •

edited

Loading

erquhart Nov 13, 2018

LoicMahieu Nov 21, 2018

erquhart Nov 22, 2018

erquhart commented Nov 22, 2018

talves commented Nov 22, 2018

erquhart commented Nov 22, 2018

talves commented Nov 22, 2018 •

edited

Loading

erquhart commented Nov 22, 2018

talves commented Nov 24, 2018

erquhart commented Nov 26, 2018

LoicMahieu commented Dec 5, 2018 •

edited

Loading

erquhart left a comment

erquhart Feb 2, 2019 •

edited

Loading

talves commented Feb 2, 2019

erquhart commented Feb 2, 2019

erquhart commented Feb 5, 2019

		@@ -111,12 +106,15 @@ export function loadConfig() {
		try {
		const preloadedConfig = getState().config;

Skip load config.yml config #1866

Skip load config.yml config #1866

Conversation

LoicMahieu commented Nov 9, 2018

verythorough commented Nov 9, 2018 • edited Loading

talves commented Nov 9, 2018 • edited Loading

LoicMahieu commented Nov 9, 2018

verythorough commented Nov 9, 2018 • edited Loading

erquhart Nov 13, 2018

Choose a reason for hiding this comment

LoicMahieu Nov 21, 2018

Choose a reason for hiding this comment

erquhart Nov 22, 2018

Choose a reason for hiding this comment

erquhart commented Nov 22, 2018

talves commented Nov 22, 2018

erquhart commented Nov 22, 2018

talves commented Nov 22, 2018 • edited Loading

erquhart commented Nov 22, 2018

talves commented Nov 24, 2018

erquhart commented Nov 26, 2018

LoicMahieu commented Dec 5, 2018 • edited Loading

erquhart left a comment

Choose a reason for hiding this comment

erquhart Feb 2, 2019 • edited Loading

Choose a reason for hiding this comment

talves commented Feb 2, 2019

erquhart commented Feb 2, 2019

erquhart commented Feb 5, 2019

Skip load `config.yml` config #1866

Skip load `config.yml` config #1866

verythorough commented Nov 9, 2018 •

edited

Loading

talves commented Nov 9, 2018 •

edited

Loading

verythorough commented Nov 9, 2018 •

edited

Loading

talves commented Nov 22, 2018 •

edited

Loading

LoicMahieu commented Dec 5, 2018 •

edited

Loading

erquhart Feb 2, 2019 •

edited

Loading