feat(lib): updates code to use es6 and promise based async #231
Conversation
UlisesGascon
left a comment
There was a problem hiding this comment.
I will let this PR open for other reviewers... I plan to land this PR next week. Great refactor @mfdebian 👏
| - Node.js 10.x | ||
| - Node.js 12.x | ||
| - Node.js 14.x | ||
| - Node.js 16.x |
There was a problem hiding this comment.
In a separate PR we can add support for newer versions too :)
There was a problem hiding this comment.
Will work on this during the weekend! 🫡
bjohansebas
left a comment
There was a problem hiding this comment.
There are style changes, it's a big PR, I'd like to review it more carefully
|
@bjohansebas I plan to land this next week. I did an online review with @mfdebian of the changes as this PR was quite extensive. Let me know if you want us to organize a similar session with you 👍 |
bjohansebas
left a comment
There was a problem hiding this comment.
Okay, this is my first review. Just a small comment that isn’t very important. I’ll continue reviewing it tomorrow
|
|
||
| return res; | ||
| }; | ||
| const obj = Object.create(null); |
There was a problem hiding this comment.
it basically prevents an error from being thrown if someone does delete Object.create
| const obj = Object.create(null); | |
| const obj = { __proto__: null }; |
bjohansebas
left a comment
There was a problem hiding this comment.
LGTM, great work.
I think it would be great if in the next version you stopped supporting Express v3, and maybe started creating tests for Express v5. Although in principle they should be the same tests as Express v4, so they could be reused for Express v5.
| } | ||
| } | ||
|
|
||
| // TODO check if layout path has .hbs extension |
There was a problem hiding this comment.
this TODO is complete with this line:
Line 307 in bd43246
|
Note that when making linter changes that affect a large portion of the code, it's better to do them in a separate PR and add |
This PR adds a bunch of changes to the core
libfiles but basically does 2 things asked for in #203:it also makes the code
es6compliant.I've intentionally not touched any tests (besides replacing
var) and not changed anything in the intended behavior of the library.