search

datalust / seq-logging

A Node.js client for the Seq HTTP ingestion API
Apache License 2.0
25 stars 15 forks source link

Browser support without user-provided polyfills #61

Closed ymotton closed 1 year ago

ymotton commented 1 year ago

Ever since webpack 4->5 removed node polyfills for builtin packages like url, http and https, there are a number of hoops you need to jump through in order to be able to use packages that use them in the browser.

Any chance you are considering a browser-native version of the client? I suppose you could release a client that just uses the polyfills locally?

liammclennan commented 1 year ago

Hi @ymotton,

I suspect we ended up here because seq-logging was designed for Node and not originally intended to run in the browser. A better client-side experience would certainly be interesting if anyone would like to look at it.

ymotton commented 1 year ago

@liammclennan I'm fine forking a version specifically for the browser if you'll maintain it as the product evolves?

liammclennan commented 1 year ago

It would be better not to maintain two copies of the same library if it can be modified to work nicely in both environments.

It is probably best for seq-logging to depend on browser APIs and be polyfilled on Node.

u-rogel commented 1 year ago

@liammclennan I would like to use this tool as well both on browser and node. Since node v18 we can use fetch in node as well. Will switching to fetch would be an approach you are willing to follow?

If yes I would gladly help.

u-rogel commented 1 year ago

@liammclennan migrating away from Buffer was more of an issue but possible as well in node v18. Submitted a draft for a PR #64. Looking forward for your feedback.

liammclennan commented 1 year ago

@u-rogel thank you for the PR. Do you think it could include a fetch polyfill so that it can work in node < 18? Otherwise we have to bump the major version. Most users of seq-logging are via one of the other libraries (pino-seq, winston-seq etc) and they won't get the upgrade.

liammclennan commented 1 year ago

I've run into another wrinkle. require('buffer').Blob does not exist in Node 12.

KodrAus commented 1 year ago

Looks like Node 12 is over a year past EOL now based on these release notes:

The 12.x release line now moves into "Active LTS" and will remain so until October 2020. After that time, it will move into "Maintenance" until end of life in April 2022.

If it's too troublesome we may be able to drop support for it. What do you think?

liammclennan commented 1 year ago

seq-logging will drop support for Node 12 in version 2.0.0.

u-rogel commented 1 year ago

Hi @liammclennan @KodrAus I would like to make a PR which will resolve the problem of bundling this module with webpack for browsers and also will not require the usage of node-fetch in node environment. Would it be something you would consider to merge?

liammclennan commented 1 year ago

@u-rogel what is your plan?

u-rogel commented 1 year ago

I want to use Rollup compile time conditionals to select the relevant requires for the environment. Then to export two different bundles one for node, one for browser from the same codebase. I was busy the last two months, but can write now a quick PR that we can talk about if you want.

KodrAus commented 1 year ago

@u-rogel if possible I think we should avoid needing to use a bundler or adding build steps for this library. Is the problem you're solving just support for fetch pre Node 18?

u-rogel commented 1 year ago

The current version of the bundle prevent it from being used with many features of the standard js JS bundlers, since they are hoisting any require / import they find to the top of the fail. In our case my vue and react projects bundlers are failing since 'node-fetch' is not installes. Specific output for each environment will solve it. Then browsers will not even get the 'node-fetch' require statement. Node 18 fetch will be solved by it, but that is not my goal.

I will make a draft PR later, so you can take a look.

KodrAus commented 1 year ago

Hi @u-rogel. Please feel free to submit a PR to show what you mean, but at this stage I’m against any introduction of build-time tooling to the library. I don’t think the additional complexity, debt, and constraints JavaScript bundlers introduce is worth it just to save a dependency, but I’m open to exploring options to see how they come out.

liammclennan commented 1 year ago

Hi @u-rogel

Thanks for your patience. I've taken your idea of building separate packages, but instead introduced a separate entry-point for browser consumers (seq-logging/browser). Would this address the issue you've been having?

https://github.com/datalust/seq-logging/pull/71

liammclennan commented 1 year ago

This is now merged to main