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

Would not run #27

Closed
wants to merge 1 commit into from
Closed

Would not run #27

wants to merge 1 commit into from

Conversation

sremiger1
Copy link

Had to update too the following lines to get things to run
1 Missing Package
2. Incorrect require command
3. I think this is for older node versions but the? caused an error and had to change to a 2-step compare.

@@ -14,6 +14,7 @@
"body-parser": "^1.20.0",
"dotenv": "^16.0.0",
"express": "^4.18.1",
"fs.promises": "^0.1.2",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for the PR!

So, fs.promises appears to be this package: https://github.com/cravler/fs.promises

whereas this repo is using the NodeJS Promises API
https://nodejs.org/api/fs.html#promises-api

They are different, and this project does not require the fs.promises package. I was able to get the project to run as-is without making these changes. As fs.promises is not the mainline way to use the promises API and does not appear to be maintained we do not want to create a dependency on it.
Based on your comment it appears that you are using a different version of node than the one recommended in the readme -- perhaps try that one and see if that works, and/or provide the error message you're getting?

Copy link
Author

Choose a reason for hiding this comment

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

I am using Node v12.18.1 unfortunately, I did not want to upgrade since the project I am on is what version the company is using.

I just wanted to see how this works, based on the YT video.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm... yeah, I don't think this will run on Node 12. Have you considered using a tool like nvm? That would allow you to easily install and switch between different node versions so you can try projects like this while still keeping the older version around for your company's project.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To close the loop, I confirmed that the syntax for importing fs/promises changed between Node 12 and Node 14, which explains the issue. nodejs/node#35740

Going to close this PR since this code should work correctly on more recent versions of node.

@@ -241,7 +241,7 @@ app.post("/server/receive_webhook", async (req, res, next) => {
const errorHandler = function (err, req, res, next) {
console.error(`Your error:`);
console.error(err);
if (err.response?.data != null) {
if (err.response != null && err.response.data != null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this change shouldn't be necessary if you're using the recommended version of node, but I'm all for making this project compatible with more node versions, so it seems fine

Copy link
Collaborator

Choose a reason for hiding this comment

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

It does make it inconsistent with the rest of the code base, tho.

@phoenixy1 phoenixy1 closed this Dec 14, 2023
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