noOverwriteGlobs will not work in majority of cases in templates using react

[derberg]

After we introduced react engine for templates rendering, this code was not changed

async shouldOverwriteFile(filePath) {
    if (!Array.isArray(this.noOverwriteGlobs)) return true;
    const fileExists = await exists(path.resolve(this.targetDir, filePath));
    if (!fileExists) return true;

    return !this.noOverwriteGlobs.some(globExp => minimatch(filePath, globExp));
  }

the problem is that input here, filePath is always the filePath that includes a file name as it is called in the template.

In nunjuck engine things are easy, we practice to name template files exactly as they are named after they are rendered in output. So for example in html-template we would call a file in template directory by: index.html

In react, you have more flexibility, you can follow the same approach as in nunjuck engine, have separate files templated with react, but you will call the same file I mentioned above index.html.js

Problem

So the code I mentioned above will never work in above mentioned cases as at start the filePath will be index.html.js.
When I use generator in CLI for example, I want to pass --no-overwrite="index.html" as I want to make sure it do not overwrite index.html, I do not want to remember to ignore index.html.js. And even if I remember, it will not work as const fileExists = await exists(path.resolve(this.targetDir, filePath)); will anyway return true and override my file cause index.html.js do not exists in output.

Solution

I think a bigger refactor is needed

• noOverwriteGlobs as it is, should accept only patterns/names that I have in output
• shouldOverwriteFile should not rely on filenames from template. Fix is not that easy as just remove .js from the filename as can be that template just contains index.js for example.

Some investigation is needed. Probable we should move away from generateFile the usage of shouldOverwriteFile and move it to renderAndWriteToFile where for nunjucks we do it old way and for react, we grab final name of file in output somehow from react 🤔

[lmgyuan]

How about we create a mapping system that understands the relationship between a template file and its intended output file. This could be a configuration file or part of the template metadata. The shouldOverwriteFile function would reference this mapping to determine the actual output file name to check against the noOverwriteGlobs. Would this be a good idea to solve this issue? @derberg

[Gmin2]

I was thhinking of this solution:

• Modify the shouldOverwriteFile method to accept a file name without extension
• Update the loadTemplateConfig method to parse noOverwriteGlobs as an array of patterns/filenames without extension
• Update the usage of shouldOverwriteFile to pass the fileName instead of filePath
• Update the renderReact method to get the final file name in output and pass it to the save function

[lmgyuan]

May I ask what the naming convention is in rendering the template output? If the name before the extension is not modified during rendering (which is my understanding based on the codes), we can just compare the name before the ".js" and ".html" to determine whether we should overwrite the current file. Otherwise, we might need to find a way to know what file the template input ".js" and ".jsx" files will generate, which would be more complicated.

I think this is where the filename could be modified?

if (renderedContent.metadata.fileName !== undefined) {
  const newFileName = renderedContent.metadata.fileName.trim();
  const basepath = path.basename(filePath);
  filePath = filePath.replace(basepath, newFileName);
}

But I don't find where the metadata.fileName could be changed. Maybe that's something that could be specified in the template input file? If that's the case then we probably need to find a way to know what files a template input file would generate to check whether a file can be overwritten.

[derberg]

first we need to remember there are users for generator, that do not code templates, they just use them - and therefore do not need to know how they are structured internally. Like with any scenario, where you use a library, and you just care about it's API, and not the actual implementation. So first assumption must be that when users do --no-overwrite="index.html", then do not know if the source file was index.html.js or index.js or whatever.js.html.js (in react render engine, the file name is taken from the <File/> component used inside the file). When users do --no-overwrite="index.html", they do it because they initially generated index.htmlusing the template, and after a week or 2, after updating AsyncAPI document, they want to regenerate output, all files, except ofindex.html`

this is why I'm thinking that probably the only way to make it work is to break into the process when we start writing the file, the renderAndWriteToFile function

I don't think it is possible to do a change in renderReact as the filePath we have access to there, is the path to a template, not output. So we have to accept that event if noOverwriteGlobs is defined, we will have to push all files through rendering. Then the could make a change in saveContentToFile as @lmgyuan suggests, because after react rendering, the metadata that we have access to (that comes from react-sdk) keeps the fileName of the actual output filename. Wouldn't it be best to actually just make sure saveRenderedReactContent has access to defined noOverwriteGlobal and just basing on renderedContent.metadata.fileName skip files that match the pattern?

[lmgyuan]

@derberg Yes! That sounds like a great idea and does not seem to add too much complexity to the codebase. I think following changes might be needed:

saveContentToFile in the react.js should be the place where we check whether we should save a specific content to file. We can add these codes at the end:

// get the final file name of the file
const finalFileName = path.basename(filePath);
// check whether the filename should be ignored based on user's inputs
const shouldOverwrite = !noOverwriteGlobal.some(globExp => minimatch(filePath, globExp)); 

// Write the file only if it should not be skipped
  if (shouldOverwrite) {
    await writeFile(filePath, content, {
      mode: permissions
    });
  } else {
    console.log(`Skipping overwrite for: ${filePath}`);
  }
};

I thought about how to pass the noOverwriteGlobal to the method. I think the easiest way is to use global.noOverwriteGlobal = this.noOverwriteGlobs; in the generator.js.

If we don't want to create a such a global object, we can also try something like

const setNoOverwriteGlobal = (globs) => {
  noOverwriteGlobal = globs;
};

const getNoOverwriteGlobal = () => noOverwriteGlobal;

module.exports = { setNoOverwriteGlobal, getNoOverwriteGlobal };

We can also just pass the noOverwriteGlobs as a parameter to the saveContentToFile. This can be done by modifying saveRenderedReactContent so it takes the this.noOverwriteGlobs and passes it to saveContentToFile.

Let me know what you all think! @derberg @utnim2 Thank you so much @derberg for the information about saveContentToFile!

[derberg]

sounds good, but yes, please avoid global objects and rather do composition and simply pass needed variables to functions that need it as argument

[lmgyuan]

sounds good, but yes, please avoid global objects and rather do composition and simply pass needed variables to functions that need it as argument

Should I go ahead with a PR, or do you think we need more comments before we proceed? :)

[derberg]

@lmgyuan please go ahead if you are interested 🙏🏼 I think everything that we need is clarified. This issue is here for some time already, so whoever had time to comment, already did. Thanks!

[Gmin2]

Should I go ahead with a PR, or do you think we need more comments before we proceed? :)

Proceed with the PR @lmgyuan, If you need any help/suggestion feel free to drop a comment here

[lmgyuan]

just opened a PR #1191 @derberg @utnim2