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.

Sign up for GitHub

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

Warn when using default node or yarn versions #1401

Merged
merged 2 commits into from
Nov 8, 2023
Merged

Warn when using default node or yarn versions #1401

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
012876f
Warn when using default node or yarn versions
Nov 8, 2023
f441211
Add changelog notes
Nov 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

## Main (unreleased)

- Warn when relying on default Node.js or Yarn versions (https://github.com/heroku/heroku-buildpack-ruby/pull/1401)
- Warn when default Node.js or Yarn versions change (https://github.com/heroku/heroku-buildpack-ruby/pull/1401)

## v261 (2023/11/02)

- JRuby 9.4.5.0 is now available
Expand Down
13 changes: 7 additions & 6 deletions lib/language_pack/helpers/nodebin.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,20 @@
require 'json'

class LanguagePack::Helpers::Nodebin
NODE_VERSION = "20.9.0"
YARN_VERSION = "1.22.19"

def self.hardcoded_node_lts
version = "20.9.0"
{
"number" => version,
"url" => "https://heroku-nodebin.s3.us-east-1.amazonaws.com/node/release/linux-x64/node-v#{version}-linux-x64.tar.gz"
"number" => NODE_VERSION,
"url" => "https://heroku-nodebin.s3.us-east-1.amazonaws.com/node/release/linux-x64/node-v#{NODE_VERSION}-linux-x64.tar.gz"
}
end

def self.hardcoded_yarn
version = "1.22.19"
{
"number" => version,
"url" => "https://heroku-nodebin.s3.us-east-1.amazonaws.com/yarn/release/yarn-v#{version}.tar.gz"
"number" => YARN_VERSION,
"url" => "https://heroku-nodebin.s3.us-east-1.amazonaws.com/yarn/release/yarn-v#{YARN_VERSION}.tar.gz"
}
end

Expand Down
40 changes: 40 additions & 0 deletions lib/language_pack/ruby.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see where default_node_version (or default_yarn_version) is ever written to?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

def fetch(key)
return read(key) if exists?(key)
value = yield
write(key, value.to_s)
return value
end

Copy link
Member

@edmorley edmorley Nov 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if version != version
warn(<<~WARNING, inline: true)
Default version of Node.js changed (#{old_version} to #{version})
WARNING
end

warn(<<~WARNING, inline: true)
Installing a default version (#{version}) of Node.js.
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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in #1402


https://devcenter.heroku.com/articles/ruby-support#node-js-support
WARNING

[@node_installer.binary_path]
else
[]
Expand All @@ -1028,6 +1048,26 @@ def add_yarn_binary
return [] if yarn_preinstalled?

if Pathname(build_path).join("yarn.lock").exist? || bundler.has_gem?('webpacker')

version = @yarn_installer.version
old_version = @metadata.fetch("default_yarn_version") { version }

if version != version
warn(<<~WARNING, inline: true)
Default version of Yarn changed (#{old_version} to #{version})
WARNING
end

warn(<<~WARNING, inline: true)
Installing a default version (#{version}) of Yarn
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:

https://devcenter.heroku.com/articles/ruby-support#node-js-support
WARNING

[@yarn_installer.name]
else
[]
Expand Down
3 changes: 3 additions & 0 deletions spec/hatchet/node_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@
expect(app.output).to include("bin/node is the node directory")
expect(app.output).to_not include(".heroku/node/bin/node is the node directory")

expect(app.output).to include("Installing a default version (#{LanguagePack::Helpers::Nodebin::YARN_VERSION}) of Yarn")
expect(app.output).to include("Installing a default version (#{LanguagePack::Helpers::Nodebin::NODE_VERSION}) of Node.js")

expect(app.run("which node")).to match("/app/bin/node") # We put node in bin/node
expect(app.run("which yarn")).to match("/app/vendor/yarn-") # We put yarn in /app/vendor/yarn-
end
Expand Down