Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Node modules are not installed #332

Closed
coffee-cup opened this issue Mar 8, 2022 · 6 comments · Fixed by paketo-buildpacks/nodejs#561
Closed

Node modules are not installed #332

coffee-cup opened this issue Mar 8, 2022 · 6 comments · Fixed by paketo-buildpacks/nodejs#561

Comments

@coffee-cup
Copy link

What happened?

  • What were you attempting to do?

Build and run a simple NodeJS application that uses npm

  • What did you expect to happen?

npm install was run and all node modules were installed

  • What was the actual behavior? Please provide log output, if possible.

No node modules were installed. It looks like if no start script is found in the package.json file then npm-install buildpack is not included.

Buildpacks included are

3 of 8 buildpacks participating
paketo-buildpacks/ca-certificates 3.0.3
paketo-buildpacks/node-engine     0.12.1
paketo-buildpacks/node-start      0.7.1

The run logs when starting the container is

node:internal/modules/cjs/loader:936
throw err;
^
Error: Cannot find module 'fastify'
Require stack:
- /workspace/index.js
at Function.Module._resolveFilename (node:internal/modules/cjs/loader:933:15)
at Function.Module._load (node:internal/modules/cjs/loader:778:27)
at Module.require (node:internal/modules/cjs/loader:1005:19)
at require (node:internal/modules/cjs/helpers:102:18)
at Object.<anonymous> (/workspace/index.js:1:17)
at Module._compile (node:internal/modules/cjs/loader:1103:14)
at Object.Module._extensions..js (node:internal/modules/cjs/loader:1155:10)
at Module.load (node:internal/modules/cjs/loader:981:32)
at Function.Module._load (node:internal/modules/cjs/loader:822:12)
at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:77:12) {
code: 'MODULE_NOT_FOUND',
requireStack: [ '/workspace/index.js' ]
}

If a start script is include then the buildpacks participating are

paketo-buildpacks/ca-certificates 3.0.3
paketo-buildpacks/node-engine     0.12.1
paketo-buildpacks/npm-install     0.8.0
paketo-buildpacks/node-module-bom 0.2.2
paketo-buildpacks/npm-start       0.8.0

and node modules are installed

Build Configuration

  • What platform (pack, kpack, tekton buildpacks plugin, etc.) are you
    using? Please include a version.

pack version 0.24.0+git-79a40b7.build-3148

  • What buildpacks are you using? Please include versions.
paketo-buildpacks/ca-certificates 3.0.3
paketo-buildpacks/node-engine     0.12.1
paketo-buildpacks/node-start      0.7.1
  • What builder are you using? If custom, can you provide the output from pack inspect-builder <builder>?

paketobuildpacks/builder:full

  • Can you provide a sample app or relevant configuration (buildpack.yml,
    nginx.conf, etc.)?

I've created a sample app at https://github.com/coffee-cup/paketo-npm-install-example

@ryanmoran
Copy link
Member

This is working as expected since we merged paketo-buildpacks/npm-start#179. Can you explain why you expect this to work without specifying a start command?

@coffee-cup
Copy link
Author

coffee-cup commented Mar 9, 2022

Some of the NodeJS apps we build use Procfiles or are started with custom docker start commands rather than using a package.json start command. The build looks like it will work as the node-engine and procfie buildpacks both match, but the deps are not installed so the app fails to start.

===> DETECTING
4 of 8 buildpacks participating
paketo-buildpacks/ca-certificates 3.1.0
paketo-buildpacks/node-engine     0.12.1
paketo-buildpacks/node-start      0.7.1
paketo-buildpacks/procfile        5.1.0

I've added an example on this branch

It is also a bit strange how the npm behaviour does not match the yarn behaviour

@SaschaSchwarze0
Copy link

+1 on this, and yes @ryanmoran it was caused by the PR you referenced, but not intended (@dalbar is a colleague of me). We also just stumbled over that the npm-start buildpack requests nodemodules. And now as it does not participate anymore, it does not request it anymore and npm-install does not jump in.

I think, conceptually it would make sense to decouple this. npm-install should imo always contribute if there is a package.json which has a non-empty dependencies or devDependencies section (depending on NODE_ENV). By default only for runtime. node-run-script would request it for build time as it does today.

Not sure what the best way is to get to this behavior.

@dalbar
Copy link

dalbar commented Mar 16, 2022

+1 on this, and yes @ryanmoran it was caused by the PR you referenced, but not intended (@dalbar is a colleague of me). We also just stumbled over that the npm-start buildpack requests nodemodules. And now as it does not participate anymore, it does not request it anymore and npm-install does not jump in.

I think, conceptually it would make sense to decouple this. npm-install should imo always contribute if there is a package.json which has a non-empty dependencies or devDependencies section (depending on NODE_ENV). By default only for runtime. node-run-script would request it for build time as it does today.

Not sure what the best way is to get to this behavior.

@ryanmoran Wouldn't it be sufficient to add an additional build group in https://github.com/paketo-buildpacks/nodejs/blob/main/buildpack.toml that install npm dependencies and runs node-start with a lower priority than the current npm-start group.

Currently the build groups are: yarn stuff, npm-install+npm-start and node-start.
With changes: yarn stuff, npm-install+npm-start, npm-install+node-start and node-start.

Ordered groups should lead to the desired behavior.

dalbar added a commit to dalbar/nodejs that referenced this issue Mar 22, 2022
Runs `node-start` right before `npm-start` in the npm group.

Fixes paketo-buildpacks/npm-install#332
dalbar added a commit to dalbar/nodejs that referenced this issue Mar 22, 2022
Runs `node-start` right before `npm-start` in the npm group.

Fixes paketo-buildpacks/npm-install#332
@ryanmoran
Copy link
Member

This PR (paketo-buildpacks/node-start#203) will allow the node-start buildpack to pull in node_modules if needed. Once this lands, we can add node-start to both the yarn and npm order groups in the node buildpack.toml. We can then also make their respective *-start buildpacks optional. At this point, I believe we will have restored the original behavior.

@dalbar I'd also love to see a PR to yarn-start that replicates the behavior that we have today in npm-start with regard to the detection of the start command.

@dalbar
Copy link

dalbar commented Mar 22, 2022

@ryanmoran sounds like a solid plan. I will work on the yarn-start changes.

dalbar added a commit to dalbar/nodejs that referenced this issue Apr 6, 2022
Runs `node-start` right before `npm-start` in the npm group.

Fixes paketo-buildpacks/npm-install#332
ryanmoran pushed a commit to paketo-buildpacks/nodejs that referenced this issue Apr 6, 2022
Runs `node-start` right before `npm-start` in the npm group.

Fixes paketo-buildpacks/npm-install#332
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants