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

[close #1202] Add support for Rails 7 & jsbundling #1212

Merged
merged 2 commits into from
Oct 19, 2021

Conversation

schneems
Copy link
Contributor

Rails 7 introduced the gem:

gem "jsbundling-rails", "~> 0.1.0"

Which no longer relies on webpacker.

Issue #1202 was opened since developers using jsbundling would expect yarn to be available but won't have the webpacker gem which we use to gate detection for yarn installation.

With this change we now install nodejs when a package.json is detected and yarn when yarn.lock is detected.

Rails 7 introduced the gem:

```
gem "jsbundling-rails", "~> 0.1.0"
```

Which no longer relies on `webpacker`. 

Issue #1202 was opened since developers using jsbundling would expect `yarn` to be available but won't have the `webpacker` gem which we use to gate detection for yarn installation.

With this change we now install nodejs when a `package.json` is detected and yarn when `yarn.lock` is detected.
@schneems schneems force-pushed the schneems/jsbundling-support branch from dbfd02e to dacd7ac Compare October 13, 2021 19:21
@schneems schneems marked this pull request as ready for review October 13, 2021 20:00
@schneems schneems requested a review from a team as a code owner October 13, 2021 20:00
@schneems schneems enabled auto-merge (squash) October 13, 2021 20:10
@edmorley
Copy link
Member

If I'm reading the code correctly, both before and after this change, if an app uses heroku/nodejs and has that buildpack before the Ruby buildpack, then the Ruby buildpack will never install its own copy of Node.

However for apps that either:

  1. Use heroku/nodejs and have it after heroku/ruby
  2. Or, don't actually use the Node buildpack and just happen to have a package.json lying around (eg for another purpose, eg other tools)

...then the Ruby buildpack will start installing its own Node, when it didn't before - even if they aren't actually using Rails 7.

In the case of (1), this means there will be two Node.js versions installed. Will this cause any problems? PATH order? Slug size?

In the case of (2) only one Node.js version will be installed, but it may/may not be unused depending on whether they use this Rails 7 feature. For apps not using Rails 7 this seems like it will increase slug size unnecessarily?

You'll know whether the above is an issue or not, but wanted to double check these had been considered :-)

@schneems
Copy link
Contributor Author

When we move over to cloud native buildpacks all apps that have a package.json will get node installed due to the way the dependencies resolve.

Use heroku/nodejs and have it after heroku/ruby

That's extremely uncommon. We do announce that we're installing the binaries, so it should at least be debuggable to see what's going on. Hopefully (but I don't know) the heroku/nodejs version would be on the path last, so it should take precedence while that buildpack is executing.

Or, don't actually use the Node buildpack and just happen to have a package.json lying around (eg for another purpose, eg other tools)

I've had other gem maintainers ask me to special case their apps to install node and webpack the same way we install it for users of webpacker. It looks like node is ~84mb uncompressed (on my mac) from https://s3.amazonaws.com/heroku-nodebin/node/release/linux-x64/node-v12.16.2-linux-x64.tar.gz. Which is substantial. The time to download and install is negligible. But the size issue is a concern.

Customers could put their package.json in the .slugignore if they're not intending it to be used at runtime and the extra space causes issues.

For apps not using Rails 7 this seems like it will increase slug size unnecessarily?

The alternative to this check of package.json presence would be to fall back to detecting based on a specific gem presence. We could go that route but I think gating off of a standard file is a more reasonable interface than having to add gems one by one to the acceptlist. It is a behavior change, though it's one customers will be getting already when they move over to CNB backed buildpacks.

@schneems schneems force-pushed the schneems/jsbundling-support branch from dacd7ac to b60d882 Compare October 19, 2021 15:57
Rails 7 introduced the gem:

```
gem "jsbundling-rails", "~> 0.1.0"
```

Which no longer relies on `webpacker`. 

Issue #1202 was opened since developers using jsbundling would expect `yarn` to be available but won't have the `webpacker` gem which we use to gate detection for yarn installation.

With this change we now install nodejs when a `package.json` is detected and yarn when `yarn.lock` is detected.
@schneems schneems force-pushed the schneems/jsbundling-support branch from b60d882 to 61361a0 Compare October 19, 2021 16:05
@schneems schneems merged commit b94277c into main Oct 19, 2021
@schneems schneems deleted the schneems/jsbundling-support branch October 19, 2021 16:11
@dentarg
Copy link

dentarg commented Oct 31, 2021

Use heroku/nodejs and have it after heroku/ruby

That's extremely uncommon.

Why do you think that?

Out of 9 apps we have using both heroku/ruby and heroku/nodejs, 8 have the nodejs buildpack after the ruby buildpack.

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 this pull request may close these issues.

3 participants