feat(cli): Add template categories #76
Conversation
|
|
||
| test('return the correct templates and categories', () => { | ||
| expect(utils.getTemplatesByCategory()).toEqual({ | ||
| Web: expect.arrayContaining([ |
There was a problem hiding this comment.
Why do we use arrayContaining on this assertion? We check for an exact match not a partial one.
There was a problem hiding this comment.
I use arrayContaining because we don't care about the order. Does that make sense?
There was a problem hiding this comment.
Yep, just for curiosity the order is not stable?
There was a problem hiding this comment.
I think it depends on your OS because it's determined by fs.readdirSync() (or perhaps Node standardizes that). It should be the same everywhere but I suppose there's no guarantee. We could add some weights if it's problematic.
src/utils/index.js
Outdated
| return allTemplates; | ||
| } | ||
|
|
||
| const newAllTemplates = allTemplates; |
There was a problem hiding this comment.
The temporary variable is useless I think, we modify the reference of the object on the next line. We can inline the return statement with a spread operator.
There was a problem hiding this comment.
We've got an ESLint rule no-param-reassign that disallows to modify the reference object. I think it's a lot more work to inline the return statement with a spread operator in this case. I might be wrong?
There was a problem hiding this comment.
Like you want, I personally prefer to avoid mutations in general but it's only my point of view.
There was a problem hiding this comment.
So do I. However I had the feeling it might be easier to understand here. I'll compare both versions!
There was a problem hiding this comment.
It's fine actually:
{
...allTemplates,
[category]: [...(allTemplates[category] || []), name],
}
src/cli/index.js
Outdated
| const templatesByCategory = getTemplatesByCategory(); | ||
| const choices = []; | ||
|
|
||
| for (const [category, values] of Object.entries(templatesByCategory)) { |
There was a problem hiding this comment.
You can also use a reduce rather than the for loop.
There was a problem hiding this comment.
I first wanted to use reduce but it seemed like it was too much compared to this loop. I will compare them again.
There was a problem hiding this comment.
This would be the equivalent:
Object.entries(templatesByCategory).reduce(
(templates, [category, values]) => [
...templates,
new inquirer.Separator(category),
...values,
],
[]
);Not sure it more readable though.
There was a problem hiding this comment.
Never mind it's not a big deal.
This PR adds categories to templates. ## Why 1. Order the templates: as we're getting more templates, it's better to distinguish web templates from mobile templates. 2. Hide templates from the CLI: templates having no `category` in their `.template.js` file don't show up in the CLI. This is a way to hide them but to still compile them to the `templates` branch for CodeSandbox (for the client, helper and AutoComplete.js templates). ## Preview 
This PR adds categories to templates.
Why
1. Order the templates
As we're getting more templates, I think it's better to distinguish web templates from mobile templates.
See if
WebandNativemake more sense thanWebandMobile.2. Hide templates from the CLI
Templates having no
categoryin their.template.jsfile don't show up in the CLI. This is a way to hide them but to still compile them to thetemplatesbranch for CodeSandbox (for the client, helper and AutoComplete.js templates).Preview