Feat: loadFromUrl supports Node.js environment

[antoineol]

trafficstars

Description

This solution provides a node implementation to fetch the font file from the URL, using standard http/https modules (to avoid adding a dependency like axios).

Btw the typing of load() does not include the third parameter that allows to use loadFromUrl. I don't know who is maintaining it.

Motivation and Context

loadFromUrl assumes it is run in the browser environment only, but I needed it in Node.js environment. I tried to polyfill XHR2, but the polyfill seems to have a bug: something hangs for 120 seconds. So I provide a nodejs implementation here.

How Has This Been Tested?

The function loadFromUrl was tested with code like:

  const url = 'https://github.com/Templarian/MaterialDesign-Webfont/blob/master/fonts/materialdesignicons-webfont.ttf?raw=true';
  const res = await new Promise<ArrayBufferLike>((resolve, reject) => {
    loadFromUrl(url, (error, response) => {
      if (error) reject(error);
      else resolve(response);
    });
  });

  console.log(new TextDecoder().decode(res));

And I tested a larger workflow with something like load(url, undefined, { isUrl: true }), ensuring it returns the font with the data I expected.

Environment: node. No impact noticed on the rest of the library. All unit tests are still passing (with just 2 warnings that were already there).

Screenshots (if appropriate):

N/A

Types of changes

• [ ] Bug fix (non-breaking change which fixes an issue)
• [X] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to change)

Counted as new feature, but I'm tempted to say it's a bug, since it uses the existing API in node, which is supposed to be a supported environment. :)

Checklist:

• [X] I did npm run test and all tests passed green (including code styling checks). => 2 warnings for code styling that were already there when I forked the master.
• [ ] I have added tests to cover my changes.
• [ ] My change requires a change to the documentation.
• [ ] I have updated the README accordingly.
• [X] I have read the CONTRIBUTING document.

[yne]

I just learned that Node 18 natively support fetch()

[Connum]

I just made a PR on the fork to add a test for this.

Edit: at some point we might want to switch over to fetch completely, but I'm fine with fixing it like that for now, as Node 16 (LTS) has just reached EOL about 3 months ago. Browser-wise, all evergreen browsers support fetch now, and I'm not 100% sure, but I guess this should be polyfilled for older browsers during build anyway.

@ILOVEPIE maybe we need some kind of roadmap to keep track of things like this?

[ILOVEPIE]

@ILOVEPIE maybe we need some kind of roadmap to keep track of things like this? @Connum milestones were being used before, we can setup new ones, also we should start using the discussions tab too.

[ILOVEPIE]

@Connum we should get this reviewed and merged, once the tests are in.

[Connum]

Alternatively, instead of waiting for @antoineol, we could merge this PR and add the test with a separate PR.

[ILOVEPIE]

@Connum I think it's bests to keep tests in the same PR. I'm not too fond of the idea of committing something to master without testing it.

[Connum]

Pinging @antoineol

If we don't hear back, we should make a new PR from this PR merged with the test I added

[Connum]

Just adapted the test because it didn't pass anymore due to the changes in the names object

[Connum]

added http and https as externals to make this run with esbuild, and fixed the test that relied on a no longer existing font file.