Repository navigation
Code Readability Suggestions #91
Description
First off this project is really cool and I am trying to understand it and contribute to it.
However, I am finding the codebase significantly difficult to read.
This difficulty to me seems to extend from the heavy usage of arrays to be utilized later as function arguments.
Many of the array usage stems from entryPoint andproxyLambdaRequest which in turn get pumped into a zero-process. This process boils down to 8 function arguments. This would be a ton more readable if structuring and destructuring were used instead.
Areas where the code is very difficult to read for me begin in buildManifest.js.
Which has led to comments such as:
// get all related files (imports/requires) of this lambda
lambdas = lambdas.map(endpoint => {
/*
TRYING TO FIGURE THIS OUT
endpoint at this moment is:
[urlPath, relativeFilePath, lambdaType]
to become:
[urlPath, relativeFilePath, lambdaType, [relativeFilePath, ...dependencyTreePaths]]
*/
endpoint.push(
[endpoint[1]].concat(dependencyTree(endpoint[2], endpoint[1]))
);
return endpoint;
});
Anyway, I don't mean to be critical I just want this to be easy to help out.
Activity
nmccready commentedon Nov 1, 2019
Also see here:
https://stackoverflow.com/questions/12826977/multiple-arguments-vs-options-object#answer-12827204
Also no more than 3 arguments is possible consideration when deciding to switch to options object arg.
https://en.wikipedia.org/wiki/Rule_of_three_(computer_programming)
asadm commentedon Nov 1, 2019
I absolutely agree with this!
The project needs a cleanup. As you have found, the whole problem stems from the format of the internal manifest, which is full of arrays.
I would get to solving this and the resultant argument hell asap! (currently working on making the latest release stable)
In the meantime, if you feel like you can give it a shot please go ahead!
nmccready commentedon Nov 1, 2019
Ok, I have a PR coming soon with an additional way to deal with middlewares where most my issues stem from loading a middle ware that support dynamic swagger. Should make more sense in the PR. But I have a basic express example without zero which I would like to work with zero.
https://github.com/nmccready/swagger-express-ts-example
See how to use swagger-express-ts here .
Bottom line is the middleware needs to be loaded prior without a file based route. So I am trying to make this work dynamically by adding a middlewareHandler.
nmccready commentedon Nov 1, 2019
Another thing you might want to consider is adding lazy logging to your debug usage which would improve performance.
See:
Benchmarks of many loggers but it does show improvements with lazy logging that I implemented
Notice the Disabled levels. Thats with that log level off.
asadm commentedon Nov 19, 2019
Thanks, I will check the debug module. As for the original issue, #94 and #97 should improve this.
nmccready commentedon Nov 19, 2019
#94 looks great
Not sure if I should dive into #97 yet for understanding. What's the intent for the emitter to improve ?
asadm commentedon Nov 23, 2019
Previously, there was a callback hell between file watcher, manifest module and the router. Because on each file change we needed to generate a new manifest and then on a new manifest, we needed to tell the router to update itself (if any pages were added or removed etc).
With emitter model, we just pass these emitters and the module listens for events internally.
Would love you feedback on the updated structure here.