Skip to content

Commit

Permalink
use internal fetch as replacement for node-fetch (#3184)
Browse files Browse the repository at this point in the history
related to #2649

I was able to move to internal fetch and all tests seems fine so far.

But we have one problem with the calendar module. In the docs we have
several authentication methods and one of them is `digest`. For this we
used `digest-fetch` which needs `node-fetch` (this is not so clear from
code but I was not able to get it working).

So we have 3 options:
- remove `digest` as authentication method for calendar module (this is
what this PR does at the moment)
- find an alternative npm package or implement the digest stuff
ourselves
- use `digest-fetch` and `node-fetch` for calendar module (so they would
remain as dependencies in `package.json`)

Opinions? @KristjanESPERANTO @rejas @sdetweil @MichMich
  • Loading branch information
khassel committed Sep 9, 2023
1 parent ffdf321 commit f2957f9
Show file tree
Hide file tree
Showing 16 changed files with 24 additions and 115 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ This project adheres to [Semantic Versioning](https://semver.org/).

_This release is scheduled to be released on 2023-10-01._

> ⚠️ This release needs nodejs version > `v18`, older release have reached end of life and will not work!
### Added

- Added UV Index support to OpenWeatherMap
Expand All @@ -20,6 +22,8 @@ _This release is scheduled to be released on 2023-10-01._

### Removed

- **Breaking Change**: Removed `digest` authentication method from calendar module (which was already broken since release `2.15.0`)

### Updated

- Update roboto fonts to version v5
Expand All @@ -29,6 +33,7 @@ _This release is scheduled to be released on 2023-10-01._
- Update engine node >=18. v16 reached it's end of life. (#3170)
- Update typescript definition for modules
- Cleaned up nunjuck templates
- Replace `node-fetch` with internal fetch (#2649) and remove `digest-fetch`

### Fixed

Expand Down
20 changes: 0 additions & 20 deletions js/fetch.js

This file was deleted.

1 change: 0 additions & 1 deletion js/server_functions.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
const fs = require("fs");
const path = require("path");
const Log = require("logger");
const fetch = require("./fetch");

/**
* Gets the config.
Expand Down
10 changes: 1 addition & 9 deletions modules/default/calendar/calendarfetcher.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,7 @@
*/

const https = require("https");
const digest = require("digest-fetch");
const ical = require("node-ical");
const fetch = require("fetch");
const Log = require("logger");
const NodeHelper = require("node_helper");
const CalendarFetcherUtils = require("./calendarfetcherutils");
Expand Down Expand Up @@ -39,7 +37,6 @@ const CalendarFetcher = function (url, reloadInterval, excludedEvents, maximumEn
clearTimeout(reloadTimer);
reloadTimer = null;
const nodeVersion = Number(process.version.match(/^v(\d+\.\d+)/)[1]);
let fetcher = null;
let httpsAgent = null;
let headers = {
"User-Agent": `Mozilla/5.0 (Node.js ${nodeVersion}) MagicMirror/${global.version}`
Expand All @@ -53,17 +50,12 @@ const CalendarFetcher = function (url, reloadInterval, excludedEvents, maximumEn
if (auth) {
if (auth.method === "bearer") {
headers.Authorization = `Bearer ${auth.pass}`;
} else if (auth.method === "digest") {
fetcher = new digest(auth.user, auth.pass).fetch(url, { headers: headers, agent: httpsAgent });
} else {
headers.Authorization = `Basic ${Buffer.from(`${auth.user}:${auth.pass}`).toString("base64")}`;
}
}
if (fetcher === null) {
fetcher = fetch(url, { headers: headers, agent: httpsAgent });
}

fetcher
fetch(url, { headers: headers, agent: httpsAgent })
.then(NodeHelper.checkFetchStatus)
.then((response) => response.text())
.then((responseData) => {
Expand Down
1 change: 0 additions & 1 deletion modules/default/newsfeed/newsfeedfetcher.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
const stream = require("stream");
const FeedMe = require("feedme");
const iconv = require("iconv-lite");
const fetch = require("fetch");
const Log = require("logger");
const NodeHelper = require("node_helper");

Expand Down
51 changes: 0 additions & 51 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 1 addition & 4 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,6 @@
"dependencies": {
"colors": "^1.4.0",
"console-stamp": "^3.1.2",
"digest-fetch": "^2.0.3",
"envsub": "^4.1.0",
"eslint": "^8.48.0",
"express": "^4.18.2",
Expand All @@ -85,7 +84,6 @@
"luxon": "^1.28.1",
"module-alias": "^2.2.3",
"moment": "^2.29.4",
"node-fetch": "^2.6.12",
"node-ical": "^0.16.1",
"socket.io": "^4.7.2"
},
Expand All @@ -96,8 +94,7 @@
},
"_moduleAliases": {
"node_helper": "js/node_helper.js",
"logger": "js/logger.js",
"fetch": "js/fetch.js"
"logger": "js/logger.js"
},
"engines": {
"node": ">=18"
Expand Down
4 changes: 2 additions & 2 deletions tests/e2e/env_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,12 @@ describe("App environment", () => {
});

it("get request from http://localhost:8080 should return 200", async () => {
const res = await helpers.fetch("http://localhost:8080");
const res = await fetch("http://localhost:8080");
expect(res.status).toBe(200);
});

it("get request from http://localhost:8080/nothing should return 404", async () => {
const res = await helpers.fetch("http://localhost:8080/nothing");
const res = await fetch("http://localhost:8080/nothing");
expect(res.status).toBe(404);
});

Expand Down
2 changes: 1 addition & 1 deletion tests/e2e/fonts_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ describe("All font files from roboto.css should be downloadable", () => {

test.each(fontFiles)("should return 200 HTTP code for file '%s'", async (fontFile) => {
const fontUrl = `http://localhost:8080/fonts/${fontFile}`;
const res = await helpers.fetch(fontUrl);
const res = await fetch(fontUrl);
expect(res.status).toBe(200);
});
});
11 changes: 1 addition & 10 deletions tests/e2e/helpers/global-setup.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
const jsdom = require("jsdom");
const corefetch = require("fetch");

exports.startApplication = async (configFilename, exec) => {
jest.resetModules();
Expand Down Expand Up @@ -31,7 +30,7 @@ exports.getDocument = () => {
const url = `http://${config.address || "localhost"}:${config.port || "8080"}`;
jsdom.JSDOM.fromURL(url, { resources: "usable", runScripts: "dangerously" }).then((dom) => {
dom.window.name = "jsdom";
dom.window.fetch = corefetch;
dom.window.fetch = fetch;
dom.window.onload = () => {
global.document = dom.window.document;
resolve();
Expand Down Expand Up @@ -80,14 +79,6 @@ exports.waitForAllElements = (selector) => {
});
};

exports.fetch = (url) => {
return new Promise((resolve) => {
corefetch(url).then((res) => {
resolve(res);
});
});
};

exports.testMatch = async (element, regex) => {
const elem = await this.waitForElement(element);
expect(elem).not.toBe(null);
Expand Down
4 changes: 2 additions & 2 deletions tests/e2e/ipWhitelist_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ describe("ipWhitelist directive configuration", () => {
});

it("should return 403", async () => {
const res = await helpers.fetch("http://localhost:8181");
const res = await fetch("http://localhost:8181");
expect(res.status).toBe(403);
});
});
Expand All @@ -24,7 +24,7 @@ describe("ipWhitelist directive configuration", () => {
});

it("should return 200", async () => {
const res = await helpers.fetch("http://localhost:8282");
const res = await fetch("http://localhost:8282");
expect(res.status).toBe(200);
});
});
Expand Down
4 changes: 2 additions & 2 deletions tests/e2e/port_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ describe("port directive configuration", () => {
});

it("should return 200", async () => {
const res = await helpers.fetch("http://localhost:8090");
const res = await fetch("http://localhost:8090");
expect(res.status).toBe(200);
});
});
Expand All @@ -24,7 +24,7 @@ describe("port directive configuration", () => {
});

it("should return 200", async () => {
const res = await helpers.fetch("http://localhost:8100");
const res = await fetch("http://localhost:8100");
expect(res.status).toBe(200);
});
});
Expand Down
4 changes: 2 additions & 2 deletions tests/e2e/serveronly_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,12 @@ describe("App environment", () => {
});

it("get request from http://localhost:8080 should return 200", async () => {
const res = await helpers.fetch("http://localhost:8080");
const res = await fetch("http://localhost:8080");
expect(res.status).toBe(200);
});

it("get request from http://localhost:8080/nothing should return 404", async () => {
const res = await helpers.fetch("http://localhost:8080/nothing");
const res = await fetch("http://localhost:8080/nothing");
expect(res.status).toBe(404);
});
});
2 changes: 1 addition & 1 deletion tests/e2e/template_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ describe("templated config with port variable", () => {
});

it("should return 200", async () => {
const res = await helpers.fetch("http://localhost:8090");
const res = await fetch("http://localhost:8090");
expect(res.status).toBe(200);
});
});
4 changes: 2 additions & 2 deletions tests/e2e/vendor_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,15 @@ describe("Vendors", () => {
Object.keys(vendors).forEach((vendor) => {
it(`should return 200 HTTP code for vendor "${vendor}"`, async () => {
const urlVendor = `http://localhost:8080/vendor/${vendors[vendor]}`;
const res = await helpers.fetch(urlVendor);
const res = await fetch(urlVendor);
expect(res.status).toBe(200);
});
});

Object.keys(vendors).forEach((vendor) => {
it(`should return 404 HTTP code for vendor https://localhost/"${vendor}"`, async () => {
const urlVendor = `http://localhost:8080/${vendors[vendor]}`;
const res = await helpers.fetch(urlVendor);
const res = await fetch(urlVendor);
expect(res.status).toBe(404);
});
});
Expand Down
11 changes: 4 additions & 7 deletions tests/unit/functions/server_functions_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,9 @@ describe("server_functions tests", () => {
let corsResponse;
let request;

jest.mock("node-fetch");
let nodefetch = require("node-fetch");
let fetchMock;

beforeEach(() => {
nodefetch.mockReset();

fetchResponseHeadersGet = jest.fn(() => {});
fetchResponseHeadersText = jest.fn(() => {});
fetchResponse = {
Expand All @@ -23,10 +19,11 @@ describe("server_functions tests", () => {
},
text: fetchResponseHeadersText
};
jest.mock("node-fetch", () => jest.fn());
nodefetch.mockImplementation(() => fetchResponse);
// eslint-disable-next-line
fetch = jest.fn();
fetch.mockImplementation(() => fetchResponse);

fetchMock = nodefetch;
fetchMock = fetch;

corsResponse = {
set: jest.fn(() => {}),
Expand Down

0 comments on commit f2957f9

Please sign in to comment.