Warn when using default node or yarn versions #1401
Conversation
Many developers don't realize they're using a default version. When it changes, like https://devcenter.heroku.com/changelog-items/2710, they're surprised to find that even a correctly committed `package.json` with a node engine declaration does not install the version they're expecting. This is because the Ruby build pack does not duplicate the Node build pack's version detection logic. The Ruby build pack installs a version of node for historical reasons. It pre-dates multi-build packs from when applications simply needed **a** version of node for execjs to function along with the Rails asset pipeline (sprockets). As the ecosystem has matured, our recommendations also change. We recommend not relying on this default behavior. It's worth noting that the CNB behavior (not yet available on Heroku) defers installing node to the nodes-engine build pack which is the ideal behavior so that there are not two independent paths for node support.
| This version is not pinned and can change over time, causing unexpected failures. | ||
|
|
||
| Heroku recommends placing the `heroku/nodejs` buildpack in front of | ||
| `heroku/ruby` to install a specific version of node: |
There was a problem hiding this comment.
Do we need to mention "(and define engines.node in package.json)" here too? Otherwise adding heroku/nodejs will still install a default version, just a different default (albeit using heroku/nodejs is still recommended, even if relying on default versions).
There was a problem hiding this comment.
I'm linking to the docs that mention it. I would prefer that the heroku/nodejs buildpack has a very strong recommendation and complete documentation in the output instead of duplicating it both here and in the Ruby docs. I'll be willing to reconsider with new information (or simply again at a later date).
There was a problem hiding this comment.
I agree the focus should be on "we recommend everyone use heroku/nodejs" rather than the default version part. The issue is that the wording here (a) implies that the only reason to use heroku/nodejs is if wanting to customise the version, (b) then gives slightly misleading instructions on how to customise the version (which could lead to confusion if the user doesn't follow the link).
What do you think about adjusting the wording to say roughly "we recommend you use the heroku/nodejs buildpack in front of the heroku/ruby buildpack as it offers more comprehensive Node.js support, including the ability to customise the Node.js version"?
| @@ -1018,6 +1018,26 @@ def add_node_js_binary | |||
| if Pathname(build_path).join("package.json").exist? || | |||
| bundler.has_gem?('execjs') || | |||
| bundler.has_gem?('webpacker') | |||
|
|
|||
| version = @node_installer.version | |||
| old_version = @metadata.fetch("default_node_version") { version } | |||
There was a problem hiding this comment.
I don't see where default_node_version (or default_yarn_version) is ever written to?
There was a problem hiding this comment.
The fetch with a block is a pattern in ruby:
hash = {}
one = hash.fetch("a_key") { "fallback" }
puts one.inspect
# => "fallback"
In this case it's backed by a cache, which is conceptually similar to Rails.cache#fetch https://api.rubyonrails.org/v7.1.1/classes/ActiveSupport/Cache/Store.html#method-i-fetch
The implementation for our version of fetch is here
heroku-buildpack-ruby/lib/language_pack/metadata.rb
Lines 47 to 54 in 77c1ac3
There was a problem hiding this comment.
Oh that's a really unfortunately named API.
Both the name and the appearance at the call site make it seem like the "fetch a value from this dict/hash/... and if not set fallback to value" pattern. Whereas it seems instead this API tries to simultaneously fetch a value and store a different value, under an API that name that suggests it only fetches a value.
| This version is not pinned and can change over time, causing unexpected failures. | ||
|
|
||
| Heroku recommends placing the `heroku/nodejs` buildpack in front of | ||
| `heroku/ruby` to install a specific version of node: |
There was a problem hiding this comment.
I agree the focus should be on "we recommend everyone use heroku/nodejs" rather than the default version part. The issue is that the wording here (a) implies that the only reason to use heroku/nodejs is if wanting to customise the version, (b) then gives slightly misleading instructions on how to customise the version (which could lead to confusion if the user doesn't follow the link).
What do you think about adjusting the wording to say roughly "we recommend you use the heroku/nodejs buildpack in front of the heroku/ruby buildpack as it offers more comprehensive Node.js support, including the ability to customise the Node.js version"?
Many developers don't realize they're using a default version. When it changes, like https://devcenter.heroku.com/changelog-items/2710, they're surprised to find that even a correctly committed
package.jsonwith a node engine declaration does not install the version they're expecting. This is because the Ruby build pack does not duplicate the Node build pack's version detection logic.The Ruby build pack installs a version of node for historical reasons. It pre-dates multi-build packs from when applications simply needed a version of node for execjs to function along with the Rails asset pipeline (sprockets). As the ecosystem has matured, our recommendations also change. We recommend not relying on this default behavior.
It's worth noting that the CNB behavior (not yet available on Heroku) defers installing node to the nodes-engine build pack which is the ideal behavior so that there are not two independent paths for node support.