Refactor initialization code and update typical init

[crphang]

Things Left To Do:

• Refactor based on comments on the architecture
• Update the default theme to actually reflect what is required in Update 'init' command to create a site given in markbind/init-typical #861
• Add defensive code to validate given theme
• Refactor Tests. This portion is rather major. A design doc will be updated in this thread on how unittests are refactored

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• Enhancement to an existing feature

Fixes: #861
Related to: #519

What is the rationale for this request?

This PR updates current initialization to illustrate a more complete set of features. On top of that, it also addresses the issue that editing our scaffold markbind or providing more initialization options is too complex in our current codebase which uses const hardcoded defaults.

What changes did you make? (Give an overview)

There were two main architecture that I explored in this PR on how to refactor this initialization code:

Copying the entire theme:

Creating all the files and directories, and copying from the theme if file exist for the theme:

Provide some example code that this change will affect:

The code below illustrates the change in Site.initSite based on the first architecture.

/**
 * Static method for initializing a markbind site.
 * Generate the site.json and an index.md file.
 *
 * @param rootPath
 */
Site.initSite = function (rootPath, theme) {
  const themePath = path.join(__dirname, TEMPLATE_ROOT_FOLDER_NAME, theme);

  return new Promise((resolve, reject) => {
    fs.accessAsync(rootPath)
      .catch(() => fs.copy(themePath, rootPath))
      .then(resolve)
      .catch(reject);
  });
};

Is there anything you'd like reviewers to focus on?

Design	Pros	Cons
First Design which copies everything	WYSIWYG nature can make it easy to work with for maintainers to understand the code of initialization because it is simple	The codebase will have many repetition as we need to provide empty directories for all themes/templates even if it is empty
Second Design which creates directories first and only copies required or provided files	Only need to provide the files that is required. When adding a new directory, we don't have to go into each of those themes to add those empty directory if those themes don't require.	Similar to current codebase, we have to go through the entire list of file generation to understand what files are created.

This commit illustrates the first design, but I am more for the second design which is basically adding 1 additional line to each promise blocks of the previous code. Would appreciate some discussion here.

Example second design:

function createAndCopyFileFromTheme(generateFileOrDirectory, objectPath, themePath) {
  generateFileOrDirectory
  .then(() => {
    if (!fs.existsSync(themePath)) {
      return Promise.resolve();
    }
    return fs.copyAsync(themePath, objectPath)
  })
  .catch(() => {
    return Promise.reject();
  })
}

Site.initSite = function (rootPath, theme) {
  const themePath = path.join(__dirname, TEMPLATE_ROOT_FOLDER_NAME, theme);
  const boilerplatePath = path.join(rootPath, BOILERPLATE_FOLDER_NAME);

  return new Promise((resolve, reject) => {
    fs.accessAsync(boilerplatePath)
      .catch(() => {
        if (fs.existsSync(boilerplatePath)) {
          return Promise.resolve();
        }
        const boilerplateThemePath = path.join(themePath, BOILERPLATE_FOLDER_NAME);

        return createAndCopyFileFromTheme(fs.mkdirp(boilerplatePath), boilerplatePath, boilerplateThemePath);
      })
      .then(resolve)
      .catch(reject);
  });
}

[crphang]

Request for comments

cc: @damithc

[damithc]

cc: @damithc

As this is an internal design improvement, I'm not in a position to say much. I'll wait for senior devs to weigh in.

[yamgent]

I am leaning towards the second design, although I feel that both designs will start to show their flaws when we need to rename existing directories.

Second Design Pros:
Only need to provide the files that is required.
When adding a new directory, we don't have to go into each of those themes to add those empty directory if those themes don't require.

Actually, is there strictly a need to follow a directory structure for the template in the first place? I feel that this requirement will make it harder to maintain as we now have to ensure that both the code and the template directory structure are in sync with each other, which isn't desirable either.

[crphang]

@yamgent very good points raised there, definitely there are some restrictions on both designs.

On those points, I think a mixture of first and second design can provide both flexibility and proper safeguards. I treat the code directories as a form of Interface where all templates in the future need, these directories should be minimal. If these code directories need to be renamed or changed, the impact will be minimized.

We can then follow the first design to initialize the site if those basic directories exist (passes validation) by copying the entire template files.

What do you think?

[yamgent]

On those points, I think a mixture of first and second design can provide both flexibility and proper safeguards. I treat the code directories as a form of Interface where all templates in the future need, these directories should be minimal. If these code directories need to be renamed or changed, the impact will be minimized.

We can then follow the first design to initialize the site if those basic directories exist (passes validation) by copying the entire template files.

What do you think?

Sure, let's evolve the design to that direction and see how it goes.

[crphang]

Test changes:

1. Included validation in unit test to show test for validation.
2. Leverage more on MemFS to perform template mocking.

Previous design:

Current design:

Main differences lies in Site initialization process. Previous site initialization does not require user input, so the unit test was resembling an integration test. Current site initialization requires user input and the output depends on the template that was chosen. Since this is a Unit Test, user input does not matter for us (should be covered by integration tests for future templates added), we mock the template input accordingly.

[crphang]

@yamgent wondering whether you have any insight on the failing test. It is passing locally for me, seems it could be because of the node_modules cache.

[yamgent]

@yamgent wondering whether you have any insight on the failing test. It is passing locally for me, seems it could be because of the node_modules cache.

Were you running npm 5.8.0 when you update package-lock.json? I recall the newer version of npm handling versions differently, which may cause certain dependencies to be the wrong versions and could result in different test outputs generated.

[crphang]

Ready for review. Thanks for your help @yamgent so far! I reverted to node 8 and npm 6 to get this working. Definitely some issue dependency issues there when using node 10 and upgrading our dependencies. It will cause deploy/netlify stage to fail I would attempt to reproduce in a fresh issue.

The architecture and design are stated in previous comments on how I approached this issue. Hopefully that will help in reviewing. I adopted open-close principal in allowing for developers to create new initialization templates, and also this design made it easier to integrate with remote templates in the future.

[yamgent]

I am fine with the current implementation atm.

Future extension work on this feature could allow users to use customized templates that are not shipped together with MarkBind (currently all templates must be stored in src/template/ folder, which is not accessible to MarkBind users normally). This can be explored in a future PR.

A comment: src/template/page.ejs seems a bit out of place now that the template/ folder is used for a different purpose.

[crphang]

Made changes accordingly @yamgent. Thanks for helping to review!

I was wondering where would be most appropriate to place page.ejs since template dir is being hijacked 😆.

Based on my understanding, our current logic doesn't allow multiple forms of base template which means the current structure of page.ejs is quite coupled with our design. In that direction of thought, I would think that placing it at top level together with Page.js and Site.js would be appropriate. To limit the scope of this PR, I feel that it would be better to refactor this in a separate PR. What do you think?

[yamgent]

Based on my understanding, our current logic doesn't allow multiple forms of base template which means the current structure of page.ejs is quite coupled with our design. In that direction of thought, I would think that placing it at top level together with Page.js and Site.js would be appropriate. To limit the scope of this PR, I feel that it would be better to refactor this in a separate PR. What do you think?

Yes we can do it in a separate PR, putting it together with Page.js and Site.js seems fine.

---

There is still a problem with the template overwriting existing files already on the directory. Steps to reproduce:

1. Do a markbind init on an empty directory.
2. Modify a file (e.g. modify index.md).
3. Run markbind init on the same directory.

Expected: The second run of markbind init does not touch index.md.
Actual: index.md is overwritten with the template content again and the modifications are lost.

We want the expected behaviour because authors might re-run markbind init on an existing project directory in order to add new files that weren't present in previous versions of MarkBind. So this behaviour needs to be fixed.

[crphang]

Great looking out there!

Added a fix and a unit test to check that behavior

[yamgent]

You might want to rename the first commit to follow the same as the PR title.

The second commit's title can be "Fix missing google-analytics code for test case outputs" to be more clear about what is going on (mainly fixing the test case outputs). The PR number can be mentioned in the body of the commit as additional details.

Also one minor nit:

[crphang]

Changed according to comments. Ready for review @yamgent. Thanks a lot!

[yamgent]

You might want to rename the first commit to follow the same as the PR title.

Apologies for being unclear, when I stated this, I meant removing the "[861-update-init-command]" part, so the title should only be "Refactor initialization code and update typical init". We don't put branch names in commit titles here. 😛

Otherwise this looks pretty good. 👍

[crphang]

Updated the commit name. Also merged the third commit into the first. Thanks a lot 😄