works in node and browser by u-rogel · Pull Request #64 · datalust/seq-logging

u-rogel · 2023-04-25T16:08:44Z

This is only a draft, but works.

u-rogel · 2023-04-25T17:47:44Z

I needed to replace the required modules: http & https with the fetch api which is available out of the box in node v18. Since the imported module made no difference anymore there was no need for the url module since the protocol is less of a concern.
Then next issue was the Buffer class. I saw Blob can achieve the same use case and since node v18 is available as well in node as well as in the browser.

KodrAus · 2023-04-25T22:16:16Z

Hi @u-rogel 👋

Thanks for looking into this! I've just converted the PR to a draft for you while it's still in-progress. It looks like the only lingering question left is the retrying on timeouts?

u-rogel · 2023-04-26T06:35:39Z

Reply to @liammclennan from the issue #61
Unfortunately I am not aware of any such polyfill module. Only solution I am aware of is this one - from node-fetch.
Also I don't know if that is really an upgrade for the node only module, hence might make sense to bump the version. Alternatively, would it be possible to also make a newer release for the sub-modules of winston-seq & pino-seq.

@KodrAus thanks for labelling it as draft. Yes, working out the timeouts retry is still missing. Will need to check if it can be done easily on both envs or do we need to search for some other solution.

u-rogel · 2023-04-26T08:37:00Z

Timeout handling added.
So left with fetch polyfill for node < 18. Let me know what you guys think.

liammclennan

Thanks @u-rogel. This PR is getting close. I've added some comments to address some issues and to provide support back to node 14.

liammclennan · 2023-04-27T01:53:17Z

-let url = require('url');
-
+const NodeBlob = require('buffer').Blob
+const GlobalBlob = Blob !== undefined ? Blob : NodeBlob


this fails in older node. Try typeof(Blob) !== 'undefined'

liammclennan · 2023-04-27T03:12:07Z

+                  method: "POST",
+                  headers: {
+                    "Content-Type": "application/json",
+                    "X-Seq-ApiKey": this._apiKey ? this._apiKey : null,


In my testing this is not working properly. X-Seq-ApiKey: null causes this error 'Payload from [...] specified invalid API key ·····'

How did you test it?
Do you guys using your test-suits?

liammclennan · 2023-04-27T03:27:23Z

        this._lastRemoteConfig = null;
-
-        this._httpModule = this._endpoint.protocol === "https:" ? https : http
-        this._httpAgent = new this._httpModule.Agent({


the reference to _httpAgent on line 96 causes a crash

liammclennan · 2023-04-27T03:53:47Z

-const HEADER_FOOTER_BYTES = Buffer.byteLength(HEADER, 'utf8') + Buffer.byteLength(FOOTER, 'utf8');
-
+const HEADER_FOOTER_BYTES = (new GlobalBlob([HEADER])).size + (new GlobalBlob([FOOTER])).size



fetch can be polyfilled by referencing node-fetch@2 and then something like:

const fetchApi = typeof(fetch) === 'undefined' ? require('node-fetch') : fetch;

then it works in node 16

Slightly different approach but done!

liammclennan · 2023-04-27T03:54:58Z


        return new Promise((resolve, reject) => {
            const sendRequest = (batch, bytes) => {
+                const controller = new AbortController()


To support node 14 we must handle missing AbortController.

Something like:

const controller = typeof(AbortController) === 'undefined' ? () => ({ abort: () => {} }) : new AbortController();

I think better to use the abort-controller since we can provide the same functionality.

Better naming for the polyfilled apis

u-rogel · 2023-04-27T15:20:54Z

I think I found a workaround missing packages in the browser.
Now it requires node-fetch@2 and abort-controller for the polyfills but works. I tested in the browser with an own react project, with your browser tool in the example folder and both worked. Also I tested with node 18 without polyfills and works too. Last but not least I tested with node 14 & node 16 after installing the two packages and the example script ran fine as well.
I did left for you guys to do the proper adjustments for the package.json file and also if you want to add some to the README.md or so.
Can you spot any open topic?

liammclennan · 2023-04-28T00:35:25Z

-        return this.flush().then(() => {
-            this._httpAgent.destroy();
-        });
+        return this.flush().then(() => {});


the .then(() => {}) doesn't do anything you can just:

return this.flush();

not important

u-rogel · 2023-04-28T06:15:35Z

@liammclennan thanks!
Do you know when I can expect it be available on npm?

liammclennan · 2023-04-28T06:27:43Z

@u-rogel Won't be this week. I'm still tidying up a few things, fixing the tests, and making sure it works everywhere.

As soon as that is done I will publish the package.

Thanks for this work, and for your patience through the process.

u-rogel · 2023-04-28T08:38:15Z

@liammclennan no rush, I will use my own branch till then.
Thanks as well for the process on your end. It is my first PR to an open-source project, so a bit exciting for me.

Another mini contribution is for future development, would be great to add something like this to the example folder or something similar for spinning up a seq-server:

version: '3'
services:
  seq:
    image: datalust/seq:latest
    deploy: 
      resources:
        limits:
          memory: 14G
        reservations:
          memory: 14G
    # volumes:
    #   - /datadrive:/data
    environment:
      - ACCEPT_EULA=Y
    ports:
      - 80:80
      - 5341:5341
    expose:
      - 80
      - 5341
    labels:
      - traefik.backend=seq
      - traefik.frontend.rule=Host:seq.<snip>.com
      - traefik.port=80

liammclennan · 2023-05-02T05:54:26Z

Thanks @u-rogel

Something like that might be nice.

works in node and browser

2698f5d

u-rogel mentioned this pull request Apr 25, 2023

Browser support without user-provided polyfills #61

Closed

KodrAus marked this pull request as draft April 25, 2023 22:12

implemented timeout with retry

796fef1

liammclennan previously requested changes Apr 27, 2023

View reviewed changes

u-rogel added 6 commits April 27, 2023 13:08

resolved removed agent reference

8e22066

node-fetch polyfill

38191ae

polyfill for AbortController

7145e73

Better naming for the polyfilled apis

cosmetic

2c009f0

same conditional format for all polyfills

183369b

optional polyfills with try-catch

ee540a3

liammclennan reviewed Apr 28, 2023

View reviewed changes

liammclennan marked this pull request as ready for review April 28, 2023 04:21

liammclennan merged commit 9ec7efb into datalust:dev Apr 28, 2023

cheng93 mentioned this pull request Feb 28, 2024

onRemoteConfigChange #78

Open

		const HEADER_FOOTER_BYTES = Buffer.byteLength(HEADER, 'utf8') + Buffer.byteLength(FOOTER, 'utf8');

		const HEADER_FOOTER_BYTES = (new GlobalBlob([HEADER])).size + (new GlobalBlob([FOOTER])).size

Uh oh!

Conversation

u-rogel commented Apr 25, 2023

Uh oh!

u-rogel commented Apr 25, 2023

Uh oh!

KodrAus commented Apr 25, 2023

Uh oh!

u-rogel commented Apr 26, 2023

Uh oh!

u-rogel commented Apr 26, 2023

Uh oh!

liammclennan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

u-rogel commented Apr 27, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

u-rogel commented Apr 28, 2023

Uh oh!

liammclennan commented Apr 28, 2023

Uh oh!

u-rogel commented Apr 28, 2023

Uh oh!

liammclennan commented May 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants