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

Improve changelog entry for v119 #633

Merged
merged 2 commits into from
Mar 7, 2019
Merged

Conversation

OliverJAsh
Copy link
Contributor

When we upgraded, things broke because NPM_CONFIG_PRODUCTION stopped working, however this was not a clear breaking change from the changelog. Thus, I would like to improve the changelog for other users who may also experience this.

@OliverJAsh OliverJAsh requested a review from a team as a code owner March 6, 2019 09:44
Copy link
Contributor

@jmorrell jmorrell left a comment

Choose a reason for hiding this comment

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

I think I can clarify a bit, NPM_CONFIG_PRODUCTION and YARN_PRODUCTION are not a Heroku buildpack convention. They are the env vars used by each tools themselves.

The Node buildpack did set NPM_CONFIG_PRODUCTION=true as a way of controlling npm, and a few users used this to determine that they were running on Heroku, but that was never the intended use. This led to this release causing issues for a few users.

CHANGELOG.md Outdated
@@ -82,6 +82,7 @@
## v119 (2018-02-28)

- Install and prune devDependencies by default (#519)
- Split `NPM_CONFIG_PRODUCTION` into `NPM_CONFIG_PRODUCTION` and `YARN_PRODUCTION` (for npm and Yarn respectively) (#519)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is more accurately phrased as:

No longer export NPM_CONFIG_PRODUCTION=true by default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Re. #633 (comment), I was trying to communicate that potential usage of NPM_CONFIG_PRODUCTION=false would have to be replaced with YARN_PRODUCTION=false.

@OliverJAsh
Copy link
Contributor Author

@jmorrell My perspective of this was that we were setting NPM_CONFIG_PRODUCTION=false to disable pruning of Yarn dev dependencies. This worked until we tried to upgrade. Eventually we discovered we had to replace NPM_CONFIG_PRODUCTION=false with YARN_PRODUCTION=false.

They are the env vars used by each tools themselves.

Hmm, but they seem to be used by the buildpack, no? E.g.

elif [ -n "$YARN_PRODUCTION" ]; then

@jmorrell
Copy link
Contributor

jmorrell commented Mar 6, 2019

Hmm, but they seem to be used by the buildpack, no? E.g.

Yes, if a user has set this to either true or false then pruning is unnecessary because Yarn will already have installed the desired set of dependencies the first time. This happened in release v121 though. Here's the relevant discussion:

#529
#526

@jmorrell
Copy link
Contributor

jmorrell commented Mar 6, 2019

My perspective of this was that we were setting NPM_CONFIG_PRODUCTION=false to disable pruning of Yarn dev dependencies

I don't believe this was ever the way the buildpack worked. We never checked the npm env var when choosing whether or not to prune with yarn: https://github.com/heroku/heroku-buildpack-nodejs/pull/519/files#diff-b796d034e8c512aaeb727f89e3598620R97 Can you say more about this?

@jmorrell
Copy link
Contributor

jmorrell commented Mar 6, 2019

this happened in release v121 though

I was inaccurate here. We were checking in v119, it was expanded in v121

@jmorrell
Copy link
Contributor

jmorrell commented Mar 6, 2019

As an aside: This whole change was necessary for users and eliminated a whole class of errors that used to plague support, but it was a nightmare to pull off with a minimum of disruption to existing users 😞 I'm sorry you're hitting issues updating

@OliverJAsh
Copy link
Contributor Author

I don't believe this was ever the way the buildpack worked. We never checked the npm env var when choosing whether or not to prune with yarn

Aha, you're right. We were specifying NPM_CONFIG_PRODUCTION=false but that would only have applied for npm, not for Yarn (which we use). In that case, the breaking behaviour wasn't a change in NPM_CONFIG_PRODUCTION but rather the fact the buildpack started to automatically prune Yarn dependencies. In that case the changelog makes sense as it is. Thanks for clarifying!

@jmorrell jmorrell merged commit c142018 into heroku:master Mar 7, 2019
jskrzypek added a commit to Yoobic/heroku-buildpack-nodejs that referenced this pull request Apr 2, 2019
* upstream/master:
  Update changelog (heroku#638)
  Add v140 to changelog (heroku#640)
  Add metadata logging when the build fails (heroku#630)
  Rename build-data module to metadata and update the callsites (heroku#629)
  New module for running experiments (heroku#631)
  Fix two issues with kvstore (heroku#637)
  Also print the change warning when the build fails (heroku#639)
  Add temporary warning about "run build" change (heroku#636)
  Run the build script by default (heroku#628)
  Update CHANGELOG.md (heroku#635)
  Improve changelog entry for v119 (heroku#633)
  Make breaking change warning header brighter
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.

2 participants