-
Notifications
You must be signed in to change notification settings - Fork 653
Conversation
…devDeps for build & auto-runs build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you as always for an awesome buildpack @mars 🙏
# Set env vars for the inner buildpacks in `.buildpacks` | ||
# * during compile, install build tooling (devDependencies) with npm & Yarn | ||
# * in runtime, NODE_ENV is not used (this buildpack launches a static web server) | ||
export NPM_CONFIG_PRODUCTION=false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We used to export NPM_CONFIG_PRODUCTION=true
for the Node buildpack and removed in heroku/heroku-buildpack-nodejs#519 which broke a couple of users who were using this ENV var to detect that they were building on Heroku.
Ex: heroku/heroku-buildpack-nodejs#519 (comment)
I don't think there's a way around this, just wanted to call it out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It felt real good deleting all the NPM/NODE env monkey business. Now this buildpack utilizes/acts like the Node buildpack as much as possible!
@mars I ran into an issue that I believe I've resolved, but I want to double-check that I'm doing this right. I ran Heroku's So long story short, when using the new buildpack that you will officially launch on March 11, users need to make sure they don't use the |
Thanks @ajmueller 😸 I updated the PR description with that |
if I've already added pointing this out because I wonder how much visibility this PR has for people who may have seen the log in their Heroku builds to add also pointing this out because
above is somewhat vague in terms of what changes are or are not required given previous actions taken re: the Heroku changes. |
@ekilah I understand your concern, but this is not an official Heroku buildpack so there's no further visibility I can give the change 😔
If you add both of those things to your If you add only the "Skip build on Heroku" thing to your app, then yes, it's up to you to remove that thing when it's no longer appropriate, on or after March 11. SolutionI recommend adding only Lazy work-aroundDo not make any of these heroku-postbuild or heroku-run-build-script changes to your app, and simply avoid deploying on March 11th until the new platform behavior is launched and this PR is merged & released for this buildpack. |
@mars makes sense, though unfortunate. figured I'd point it out
well, they are not conflicting as I understand it. |
@ekilah, One says "run the build script" and the other says "skip the build script". They are conflicting, and you will have to remove the "skip the build script" one in order to deploy successfully on/after March 11. The Get Ready section in the PR description above is the best way to make this transition smoothly on March 11th. If you don't want to follow Get Ready and deploy using this branch, then you don't need to do anything. On March 11, simply wait for the new platform behavior to be launched and this PR merged & released for this buildpack. |
from https://help.heroku.com/P5IMU3MP/heroku-node-js-build-script-change-faq
(I'm just clarifying here for other readers, I fully understand what needs to be done at this point. thanks for the fixes!) |
No this does not make sense. Setting both of those properties in your Once again, please follow the Get Ready section in the PR description above, or do nothing and it will continue to work after March 11. All of this confusion is just about deploying during the transition on March 11. |
I may be able to update the command line script to detect if users are using this buildpack when it runs so that it can avoid making changes. I will look into this. |
Thanks @jmorrell . Perhaps I misunderstand what you're suggesting, but I don't think we need any auto detection for this buildpack. Also, seeing your demo this morning clarified what seems to be the confusion here:
This buildpack will keep working without manual changes to your app. |
@mars What about an env var that you can set in this buildpack that will suppress the error message until it's removed? |
@jmorrell, that would be beneficial (avoid confusion) for folks using master or registry release of this buildpack, but if they're set to an older version or their own fork then they must make the updates to avoid double builds. |
@mars If I understand my own code correctly, I believe that you can add https://github.com/heroku/heroku-buildpack-nodejs/blob/master/bin/compile#L104 Though it will also show them the "successfully opted in" message 🤔 |
@jmorrell sounds great. I will verify that your |
@jmorrell I just updated this branch with the That message that you mentioned is logged:
The second line is a bit misleading in this case, because the user didn't necessarily change their I'm reluctant to push this out ahead of March 11, just because so many folks are not anticipating this change until then, but this will make it so I can release this early on March 11 and have no gap in deployability. Now updating this PR description with this new plan. |
* Use Node buildpack‘s auto build * Remove NODE_ENV=development workaround since Node buildpack installs devDeps for build & auto-runs build. * 📚 link to Dev Center for Node.js build customization * Switch to new Node auto build behavior ahead of release * Upgrade to "Node auto build" release version of inner buildpack
Resolves #155
Adapts this buildpack for the upcoming change to the official Node.js buildpack.
⛵️ No changes are required to apps deployed with the master version of this buildpack. The
npx @heroku/update-node-build-script
changes should NOT be applied to apps deployed with master version of this buildpack.Includes a few inherent changes to the architecture:
npm run build
automaticallydevDependencies
for auto build (and afterwards it prunes them), so a confusingNODE_ENV
workaround has been removedRelease Schedule
This feature will be merged & released on March 11, 2019, before the Node buildpack switches over, so that the transition to auto build is seamless.
Get Ready
If you want to test this, follow these instructions to switch to the new behavior ahead of its release. Otherwise, you do not need to do anything to continue using this buildpack before, on, or after March 11.
Set your app to use this buildpack branch:
"heroku-postbuild": "echo Skip build on Heroku"
to avoid the new auto build behavior, then make sure to remove that property frompackage.json
so that auto build can run.Finally, remember to switch the app back to the master buildpack release so that your app automatically receive buildpack updates:
Test It & Holler
Use the Get Ready instructions above to try it ahead of time and let me know if you find any problems!
Especially thank you @jmorrell for continuing to evolve the Node.js buildpack 🥓💯✅💜 and messaging so clearly about this upcoming change!