Added Feature: Build all .abell files from source #12
Conversation
There was a problem hiding this comment.
Hi Akash, This looks great! Thank you so much🐨🎉 I just request some changes in the comments. Also if there is any way to roll back the prettier changes? That would make the PR a little less bloated. It's fine if it is too complicated we can leave it like this.
EDIT: I guess prettier changes are fine, Just changing the ones mentioned below would work.
src/action.js
Outdated
| if (programInfo.logs == 'minimum') console.log(`${boldGreen('>>>')} Files built.. ✨`); | ||
| } | ||
| // GENERATE OTHER HTML FILES FROM ABELL | ||
| abellFiles.map((file) => { |
There was a problem hiding this comment.
I guess you meant to do .forEach here. .map is supposed to be use when you want to return something.
Also, if it is a plain array then I will suggest for (const file of abellFiles) {} instead
There was a problem hiding this comment.
Can you check this once again? I guess this isn't resolved
src/helpers.js
Outdated
| const files = inputFiles || fs.readdirSync(base); | ||
| let result = inputResult || []; | ||
|
|
||
| files.map(function(file) { |
There was a problem hiding this comment.
This too can be done with for (const file of files) {}
src/helpers.js
Outdated
| throw new Error( | ||
| `Something went wrong while fetching new configurations. | ||
| Save again to refresh the dev server.` | ||
| ); // eslint-disable-line |
There was a problem hiding this comment.
This will add an extra line break in terminal too I guess, since it is using ` for strings. Writing it in single line is fine since it is just an error message and not an important code.
There was a problem hiding this comment.
This won't add a new line unless a \n is added there. I split that line because eslint was complaining of the line being too long. Do you have any suggestions for that ?
There was a problem hiding this comment.
It actually does. In JavaScript when you use '`' for strings, it keeps the line breaks and additional spaces. This is one of the reasons why I had // eslint-disable-line their to disable the eslint error. And in case of strings it's fine if it crosses the line length limit because the workaround of adding \n to satisfy eslint is more unreadable than having long horizontal line
src/helpers.js
Outdated
|
|
||
| // Returns all .abell files in src folder except for [$slug] | ||
| const getAbellFiles = (sourcePath, extension) => { | ||
| const absolutePaths = recFindByExt(sourcePath, extension.split('.')[1]); |
There was a problem hiding this comment.
The right way to find an extension would be using path.extname(sourcePath) in nodejs. This code can break when you have a file something like abell.config.js where index 1 would have 'config' instead of 'js'
There was a problem hiding this comment.
I agree with path.extname there. But I would still have to split on . after getting the extension, because the function I added expects an extension without a .
There was a problem hiding this comment.
But actually, this extension is passed from programInfo, so wouldn't we explicitly want to get those files if we're passing .abell.config.js as an extension ?
There was a problem hiding this comment.
Nvm, I refactored the code so extension is passed with a . in it, and the function handles the . correctly. So no splitting or path.extname needed.
There was a problem hiding this comment.
Nvm, I refactored the code so extension is passed with a
.in it, and the function handles the.correctly. So no splitting orpath.extnameneeded.
This sounds a lot better! thanks
There was a problem hiding this comment.
This is refactored
|
Hey @saurabhdaware please check out the latest commit. Hopefully it follows your coding style. |
Hi, Akash looking great! I guess two of the changes are not resolved yet, it can you check them out? Thanks |
Fixes #8
Allows building of all .abell files in the source folder.