move function to find applications into libnodejs #8
Conversation
mhdawson
requested review from
TisVictress and
paketo-bot-reviewer
and removed request for
a team
|
This is a visual diff between the original code in node-start and the new function in libnodejs to show what's re-used which is most of the logic |
|
@thitch97 any chance you could take a look? |
find_node_application_test.go
Outdated
| }) | ||
|
|
||
| it("finds the app.js application entrypoint", func() { | ||
| Expect(os.Remove(filepath.Join(workingDir, "server.js"))).To(Succeed()) |
There was a problem hiding this comment.
@mhdawson Maybe each of these should be in its own context where only the scaffolding needed for the test is created and where it can be easily destroyed afterward? This could mean significantly more lines of code but I'm thinking about how this might be confusing to someone reading this or adding tests here in the future.
There was a problem hiding this comment.
I'm happy to adjust if we are ok with having more lines of code. Will look at doing that in next few days.
Move function from to find applications from node-start into libnodejs so that it can be used across buildpacks and extensions. API has been refined from what was in node-start to make it better as a shared function. Signed-off-by: Michael Dawson <[email protected]>
mhdawson
force-pushed
the
add-application-finder
branch
from
06fb738 to
cc37aae
Compare
|
@TisVictress pushed updated version which uses more contexts to avoid removal of files. I agree it is easier/clearer to read what the test is testing and in the end I don't think it ended up being that many more lines of code. |
|
@TisVictress ping |
|
LGTM, I'll let @TisVictress address any other concerns there might be |
Move function from to find applications from node-start into libnodejs so that it can be used across buildpacks and extensions.
API has been refined from what was in node-start to make it better as a shared function.
Summary
Add application finder functionality in node-start to libnodejs so that it can be used
across buildpacks and extensions.
Follow on PRs will
the shared function
make ubi-nodejs-extension use the shared function.
Last commit of these branches previews what those PRs will look like once this lands
mhdawson/node-start@5d6b152
https://github.com/nodeshift/ubi-nodejs-extension/commit/ce692597f5d2d5d9213f4a43b7b527850ec11ea5
Use Cases
Reduces duplicate code and centralizes knoweldge of constants
Checklist