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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions webhooks/start/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

"jose": "^4.8.1",
"js-sha256": "^0.9.0",
"jsonwebtoken": "^8.5.1",
Expand Down
4 changes: 2 additions & 2 deletions webhooks/start/server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
"use strict";
require("dotenv").config();
const fs = require("fs/promises");
const fs = require("fs.promises");
const express = require("express");
const bodyParser = require("body-parser");
const moment = require("moment");
Expand Down Expand Up @@ -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.

res.status(500).send(err.response.data);
} else {
res.status(500).send({
Expand Down