datalust / seq-logging

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

env specific files #70

Closed u-rogel closed 1 year ago

u-rogel commented 1 year ago

In general I agree with your last comment, and I think as well that less technical dept is a more maintainable approach. But please consider that in this case both node and browser users would be able to use your tool with zero headache.

As I suggested, single project, two output bundles.

When reviewing please skip the ./lib/ folder and do so later.

  1. package.json file I added a few things:

    • Added Rollup and two plugins.
    • Added build script for generating the bundles.
    • Added export property for helping bundler or node to pick up the right bundle for the env.
    • Added specific path for types.
  2. seq_logger.js I changed the former conditional requires to now be wrapped in if & else statement, specific one for each env. The conditional fetch and AbortController are for more smooth node 18 but not a must.

  3. rollup.config.js for the bundling configs. Specifying the entry-point path, the output path, the multi files plugin and the replace plugin to resolve the IS_BROWSER conditional.

Now looking again on the lib folder, you can see that the files are similar but the require calls are only there for the node bundle. I tested locally with your example files and it worked smooth for me.

KodrAus commented 1 year ago

Thanks for the PR @u-rogel :+1:

I'm a bit less against this in principle now seeing that we can keep the source working as vanilla JavaScript when the bundler simply isn't run over it (it looks like it'll just assume modern Node, which I think is the right default).

I think I'd prefer not to check the Rollup output into source though, it'll make malicious changes in PRs more difficult to spot. I'd also prefer not to use any dependencies for plugins either if possible (ones from the @rollup org are probably fine). In my experience Rollup plugins are pretty straightforward to write.

I'd like to hear what @liammclennan thinks of this though before possibly taking it any further.

u-rogel commented 1 year ago

@KodrAus I guess we can close this one, since I started an own approach. In general, yes sure I would keep the generated files out of the git repo, it was in just for you to see the output. Btw, I suggested on the other PR to publish to NPM only the files relevant for the consumer, I think it is a best practice which is very easy to have.

KodrAus commented 1 year ago

Thanks for looking into this @u-rogel :+1: It looks like we'll use the approach in #71 so will close this one in favor of it. That way we can keep all discussion centralized too.