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

Add Heroku support #34

Closed
wants to merge 3 commits into from
Closed

Add Heroku support #34

wants to merge 3 commits into from

Conversation

jgierer12
Copy link

No description provided.

@jgierer12
Copy link
Author

@watson friendly ping 🙂

@sibiraj-s
Copy link
Collaborator

is there any documentation available for this. @jgierer12 .?

@@ -12,6 +12,10 @@ Object.defineProperty(exports, '_vendors', {
exports.name = null
exports.isPR = null

function includes (str, search) {
return !!~str.indexOf(search)

Choose a reason for hiding this comment

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

Make this less clever and more explicit.

-   return !!~str.indexOf(search)
+   return str.indexOf(search) > -1;

{
"name": "Heroku",
"constant": "HEROKU",
"env": { "env": "NODE", "includes": "heroku" }

Choose a reason for hiding this comment

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

In the test, you check for this value: /app/.heroku/node/bin/node. Is that a constant? Could you check for that value explicitly instead of any value with heroku in it?

@shian15810
Copy link

shian15810 commented Jun 23, 2022

@sibiraj-s After more than a year without any update, for Heroku support, is it okay for me to create another PR to supersede this?

@sibiraj-s
Copy link
Collaborator

Thanks @shian15810 PR's always welcome. And it is good to have the approach taken is somewhere documented by Heroku. So it won't break randomly.

@sibiraj-s
Copy link
Collaborator

Sorry for the long hold. But Closing this in favour of #95.

@sibiraj-s sibiraj-s closed this Nov 11, 2022
sibiraj-s pushed a commit that referenced this pull request Nov 13, 2022
Based off of the work started in #34
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.

4 participants