loilo / livy

πŸ“œ A Monolog-inspired logging library for Node.js
MIT License
17 stars 3 forks source link

To Do items according to README.md #14

Open BenceSzalai opened 1 year ago

BenceSzalai commented 1 year ago

Hi!

I am looking for information about the project, specifically the things mentioned in the ToDo section of the README.

1.) Find an alternative for Luxon: I find it strange to hardcode such dependency into the package. Wouldn't it be better to implement timezone related processing in a optional Processor + Formatter? That would address some key points:

2.) Native Node.js ES module support: This is a natural continuation of the above topic. While on one hand Luxon could be updated from v1 to v3, which has full ES Modules support, also leaving Timezone handling to user space is a good solution for the dependency support issue. Indeed to provide a migration path for existing users a default Processor+Formatter for timezone would be needed, which should in that case probably move from luxon v1 to v3.

3.) "...might require support for top-level await in Node.js": Top level await is supported in Node.js since v14.8.0 also it has support in all major browsers since 2021, so this should really not block ES Modules support. However I don't fully see the extend of the problem or the quirks in the build chain related to the issue and what really needs to be fixed/changed to make this happen...

Long story short I really like the clean architecture with Processors, Formatters and Handlers. Most other logging libraries are a mess compared to the clarity and extensibility provided by this structure. But I am reluctant to try to utilise it in it's current form, because the mandatory timezone handling is an overkill for my usecase and the suggested lack of proper ES Module support also suggest it might become problematic.

I am willing to work on the library to improve these things if PR's are welcome. On the other hand I know the suggestion to remove the built in timezone handling is a breaking change and might not be welcome at all. But I think with the addition of a simple processor and formatter a clear upgrade path could be offered for anyone happy to use luxon as part of their logging. Basically it would only require them to include two new components in their configuration and set the timezone on these instead of setting on the logger itself.

Please let me know your thoughts, also if I have overlooked something or if i am missing a point!

Thanks!

loilo commented 1 year ago

Hi Bence!

First, thanks a lot for taking this much time to tackle the rough edges of Livy. As you may have guessed, the project has not been on top of my priorities list. The reason is as simple as it is common in Open Source: I created Livy to use it at my job at the time, then I changed jobs and now I'm working in a setting different enough that I don't need a Node.js logger anymore.

That said, having my OSS projects be up-to-date and well-rounded is still important to me, so I'm happy to see you taking the iniative on this.

You're of course right that what you suggested would be a breaking change. I'm willing to make that happen though for a couple of reasons which I'll lay out below. However, as you'll see, this goes far beyond the Luxon case alone and will be quite a lot of work:

The Luxon Dependency

You're really touching a sore point here. I had been hoping for a while now that all the datetime-related baggage would be sorted out on its own with the arrival of the Temporal API. That would basically have been Luxon-like date handling built into JavaScript.

However, while the proposal is mostly finished, it still hasn't arrived in JavaScript engines yet. (As far as I understood it, this has been due to the team behind it not only bringing this feature to JavaScript, but also are standardizing parts of it on the greater scale of IETF, where it's been advancing just slowly for over a year now.)

That said, even if the Temporal proposal was accepted tomorrow, it would still take a while to be able to broadly use it in production, so I'm afraid that this is not an option for now.

The main reason I included Luxon in Livy is that datetime formatting is at the core of many handlers, so I wanted access to that to be as close to the core as possible. A Luxon DateTime instance is more than just a JavaScript Date, it stores additional information that cannot be retreived from pure Date instance later (the said system's timezone in particular, which it receives in a rather hacky way through the Intl API), so I opted for an existing solution to store all this additional stuff instead of collecting and storing timezone-related data in my own proprietary format.

However, I deem this feature dismissble β€” actually including a timezone name in a log should be a very rare edge case, and the far more important marker (the offset of the logging machine from UTC time) is already included in the Date object.

So for Livy v2, I could imagine going along with your suggestion and store a simple JavaScript Date in log records, and then include something lean as date-fns in the handlers/formatters that do datetime formatting.

ESM Support & The Architecture

The situation around ESM support in Node.js has become much better in recent years. For version 2 of Livy, I'd probably even go so far as to only provide Livy as ESM and drop CommonJS support completely. I've been doing this with new projects recently and so far it has been working out quite nicely.

I'm aware that this can be a real pain for users stuck in a CommonJS setup, but it's a cut I'll probably have to make anyway at some point, so I'd rather do it sooner than later.

As you correctly pointed out, support for top-level await is also not something blocking this by now. While it's been available since Node 14.8, I'd actually drop Node 14 support altogether. It has been end-of-life for a couple months now and I tend to drop support for dead Node versions early β€” v2 will be the one chance of having a breaking change anyway, so I'd take the opportunity to also get rid of Node 14.

Now what's the main blocker in migrating to ESM actually is the technical setup. When I created the project, Jest (which the project uses for testing) did not have support for ESM, and it actually still doesn't (only as an experimental feature).

However, when I'm taking on a major overhaul, I'd definitively take the chance to also get rid of Jest, but also some other things: If you have ever cloned the project, you'll realize that building and testing this project is a pain, insofar as 95% of your time just goes into minutes and minutes of waiting.

This is due to some technical choices I made at the time which were reasonable back then but did not stand the test of time, in particular with regards to performance:

Now it would make for quite some research to update the project in such a way. Luckily though, I have already figured out the basics of the Vitest + Nx setup as I'm currently working on a new monorepo-based family of npm packages. It's nowhere near done yet, but I definitely can transfer the learnings from there into Livy.

So, to come back to your question…

[…] if PR's are welcome

…yes, they are, definitely!

However, I'm not willing to make the breaking change to Livy's API without also handling the other issues I mentioned above. So if you're willing to take on the task of migrating the project to Nx + Vitest, I'd be happy to see your PR β€” but this will be quite some work.

The alternative would be for me to lay the groundwork here and do the migration to Nx + Vitest myself, but without the breaking change in the Livy API. Following that, I'd be happy to see your PR for the Luxon removal.

So all in all, what we can achieve here depends a lot on your schedule.

If Livy v2 is the logger you'd love to use just in your next project that comes around next month, the v2 release will probably not make it in time for that.

If you're more broadly interested in generally using Livy as your logger of choice in Node.js, I can see this work out.

What do you think?

BenceSzalai commented 1 year ago

First of all, it is great to see your detailed response, the openness and to get a great answer this fast indeed.

I have indeed cloned the repo and looked around a bit already, but it is only to the extend to get a basic understanding of structure and few features.

The Luxon Dependency

I think I might have not yet fully grasp the scope of what Luxon is currently being used to achieve. I thought the goal was only to be able to create log entries in one timezone but write them on the output in another preset timezone. But now that you have mentioned that even the timezone is added to the output I see I need to look at it more. Anyway, no problem.

I am quite familiar with the difficulties of timezone handling and quirks and limitations of the native Date object. I think I am well positioned to figure out a simple baseline behaviour in a way that can be extended as needed.

ESM Support & The Architecture

I cannot add much to the part about dropping CommonJS support. I prefer ESM whenever I can use that. Probably many others feel the same just by looking at where things are heading in the whole ecosystem. Indeed still anyone needing CJS could use v1.

Also I know Node v14 is very outdated. I only mentioned that as that was the earliest version receiving top level await support. Indeed requiring a newer Node version once things break anyway is the reasonable approach. Do you have any particular cut-off version in mind?

Also the Readme mentions something about handling OS specific line-breaks. Could you maybe point me to the direction about what does this mean or what the issue is about? Maybe a separate issue could be opened about that if the amount of information warrants a separate context.

I am well familiar with Jest ESM support as well. I have one active project using ESM and Jest (experimental). It works quite good most the time. Mocking is painful thought compared to vitest.

Regarding Lerna however I'm not sure. According to their site, documentation and repo it is being actively maintained and developed by Nx team. I am not very familiar with these tools, but it looks like to me now Lerna uses (or can be configured to use) Nx under the hood for almost everything except for automatic package versioning and publishing. You may want to take a look at the state of the tool. I think it might have been resurrected since you last looked. Anyway, the decision to update Lerna from v3 to current v7 vs. replacing with Nx comes down to two questions afaics:

Plan and involvement

I am planing to use a great logging library for a long term project of mine. It right now uses another solution, but I'm not happy with it. The effort I'd need to put into that would be comparable to rolling my own. So instead of that I'd find it much more reasonable to contribute to a tool that is already great, only needs some love.

Now, in fact I don't really know the effort needed to make these changes. I've migrated small project before from Jest to Vitest and it was not that painful. Maybe you can take a look at current state of Lerna and see if we can keep it, as I assume migration between Lerna versions may be simpler than complete replacement with Nx.

If these are cleared I could also look at the timezone handling.

Now that being said, while I don't need a solution with a tight deadline, but I hope to replace my current logging with something better as soon as possible.

My naive assumption that few days (net) would be enough to make these changes happen and push things to an Alpha/Beta. At that point I'd set it up as my main logger, so at least I'd get additional "testing".

After that things would slow down I guess as feedback from users would be needed before moving to RC and release.

I could start working on this in August if we can agree on a plan.

loilo commented 1 year ago

I think I might have not yet fully grasp the scope of what Luxon is currently being used to achieve.

To be fair, I've also had to recall a lot of this from memory, it's been almost three years since I wrote the library, and as you probably know, undocumented reasonings vanish in far shorter time than that. πŸ˜…

As I mentioned, from some quick thoughts last night I'd assume that the Date object holds everything we really strictly need. If the timezone information is the only thing that's lost without an additional processor, so be it.

Indeed still anyone needing CJS could use v1.

I would say so as well, yes. And I'd feel like being in good company. Sindre Sorhus (aka a perceived 50% of the npm ecosystem) has dropped CJS support in new major versions of his packages in recent years, and it seems to work out quite well for him.

Indeed requiring a newer Node version once things break anyway is the reasonable approach. Do you have any particular cut-off version in mind?

I strive for supporting all Node.js versions that are officially maintained. Now, Node 16 brings us to a fuzzy decision point here as its maintenance ends at the begin of September. I could imagine cutting it off as well and add a note somewhere in the readme that Node 16 probably works but isn't officially tested and supported. My gut feeling is that while Node 16 is still widely used, it can be expected fresh projects use a more recent Node version and that Livy is added mostly to fresh projects. I'm open to feedback on this one though.

Also the Readme mentions something about handling OS specific line-breaks. Could you maybe point me to the direction about what does this mean or what the issue is about?

It's about this piece of code that imports Node's os module in a Node.js context (which it must not do in the browser for obvious reasons).

Prior to top-level await, the only way to expose OS-specific line breaks in ESM would have been an async function to conditionally do await import('os'), and honestly, having to expose an EOL character as an async function would be just ridiculous. πŸ˜… (That said, there is createRequire which probably would have solved this issue even before the availability of top-level await, but I wasn't aware of this at the time.)

I am well familiar with Jest ESM support as well. I have one active project using ESM and Jest (experimental). It works quite good most the time. Mocking is painful thought compared to vitest.

Great to hear. I'm partly open to keep Jest in the project, but from my experience (and as you also mentioned) migration to Vitest is usually quite easy and would probably be worth the effort. (Depends a lot on how mocking works in Vitest which I haven't used there yet but which is heavily used in Livy.)

While replacing Jest is not technically required for v2 (as it's not a breaking change in the API), I'd still like to complete the larger architectural changes in one go.

Regarding Lerna however I'm not sure. According to their site, documentation and repo it is being actively maintained and developed by Nx team.

Yes, I'm aware of that. I've been mostly cautious because I'm not sure for how long they will commit to maintain two separate large projects, even if one of them by now is built on top of the other. That said, Lerna is not going to stop working if maintenance is cutt off, so it's not a huge risk.

do you (want to) utilise the automatic versioning and npmjs publishing - if so Lerna should be used

I took another look and you're absolutely correct. I somehow assumed these (seemingly basic) features would also be included in Nx. Indeed I want to keep using those (and also document their usage which the project currently sorely lacks). If Lerna can utilize Nx' caching features (which it apparently can), then I'm all for keeping it.

Upgrading from v3 to v7 may be some work of its own, but it should be doable without too much time to invest.

Plan and involvement

Now there you're touching another delicate point. Just to get this straight: I'm willing to put Livy to the top of my OSS priorities, but real life goes on β€” which means I'll be on holiday for two weeks starting friday next week, and until then I'll have quite some tasks to finish for my day job. Also my daughter will start school after the summer, so there will be quite some amount of preparation to be done there as well. (Not gonna lie, my wife is going to be taking on a lot of this, but this will naturally also involve me.)

This means, before end of August, I'll only have the upcoming weekend to invest more than a couple minutes into the project (please kindly ignore that these comments also take hours to write in sum πŸ˜„), and I have no guarantees that there won't be any other unplanned real-life obligations on the weekend.

But given that this works out, I'd try to figure out the Lerna situation this weekend and take a look at how much work the Vitest migration would be.

It also crossed my mind that there are two more "architectural" issues that would need a look, so I'll also try to tackle those on the weekend:

After that things would slow down I guess as feedback from users would be needed before moving to RC and release.

Possibly. To be honest, I don't even know how many users Livy has. Inferring from npm downloads, it's not too many, and few of there would probably have any feedback to give. However, I'd definitely want to hear feedback from @donaldp as we already had a conversation about updating or replacing Luxon in Livy back in january.

So, that's the plan, although the timeline is a bit shaky. Any additional thoughts?

BenceSzalai commented 1 year ago

I'd only add that I did not expect you or assume the position to give you tasks, especially that it is not urgent for you. I thought that I'd do these things best i could, so you'd only have to review and hopefully merge. I just wanted to see your position to see if I should approach it in a collaborative way or a do it for myself way. But I think I can contribute.

I am a happy yarn-workspaces user, but npm support seems to be interesting as well.

As for TypeScript upgrade, not sure how big of a task is that, but if not like 90% of the project becomes red underline i hope it can be done too within a reasonable time.

One more thing: probably it'd worth to make a new branch, call it v1 and leave that at the current last commit should any security fixes be needed in the future on v1, while I could fire PR's against master which would progress ahead and become the nighlty -> beta -> ... -> release at some point in the future.

loilo commented 1 year ago

I'd only add that I did not expect you or assume the position to give you tasks, especially that it is not urgent for you.

Message received, thank you for being considerate. But sometimes having a user with specific needs can be a good ignition point to pursuiting the standards I'd like my Open Source projects to have anyway. 😁

I am a happy yarn-workspaces user, but npm support seems to be interesting as well.

I like to keep things as "vanilla" as possible unless an external tool adds tremendous value. Since Yarn no longer adds this value with availability of npm workspaces, I'll very likely drop it β€” omitting Yarn is simply one less hurdle for new users to take and one less though to spare for an external tool.

As for TypeScript upgrade, not sure how big of a task is that, but if not like 90% of the project becomes red underline

We're going to see. It'll probably be fine, but 4.0 to 5.1 is quite a jump. πŸ˜΅β€πŸ’«

One more thing: probably it'd worth to make a new branch, call it v1 and leave that at the current last commit

Absolutely. And after leaving v1 behind, I'd also add a note to each package's readme pointing to the v1 branch on GitHub for any users to be able to still view the v1 docs.

BenceSzalai commented 1 year ago

Hi!

I have performed some updates on my fork.

Updates:

Optimisations:

Interesting findings, notes:

Technical notes:

Typescript Compatibility

I have done the typescript upgrade in two steps: to v4.9 and to v5.1. The good news is that there were no need for any modifications to the existing code to properly compile with the newer TSC versions. I have compared the output in the lib folders after compilation with the newer TypeScript versions to those of the files built by the current upstream/master and there are no differences (except for those small changes triggered by eslint as described above). Therefore I conclude that building the project with TypeScript v5.1 results in code that must be 100% compatible with existing users.

I was thinking still to keep a separate commit for v4.9 just in case we'd like to draw a line there, as the risk of using v5.1 is in accidentally writing code that would result in d.ts files that earlier TypeScript versions would not understand. I am not sure how realistic this risk is. However right now since the TypeScript features used in the repo are from much older TypeScript, apparently it doesn't make any difference if the repo is built using v4.9 or v5.1. Imho it is fine to jump to v5.1, but even v4.9 is fine from my perspective.

Overal "v1" compatibility

All-together none of these changes should affect existing "v1" users in any way. The tooling has been modernised, the Nx caching improves the developer experience, but at it's heart this is still totally a "v1" version and I expect it to be a 100% drop-in replacement for any current users.

You can find these changes on my fork: https://github.com/loilo/livy/compare/master...BenceSzalai:livy:master

If you agree I could fire a PR for these and these changes could make it to "v1" before getting onto removing cjs support, Luxon and the other breaking changes discussed.

loilo commented 1 year ago

Oof. Okay, haven't read everything yet, will come back to the details in a comment ~later today~ tomorrow.

Just noting here that we should have coordinated this more precisely as I also have done some of these steps (Yarn replacement, Lerna Update, Node Update, TypeScript Update, Migration to test-per-package) locally already – some last week, some already back in January. Sorry, I should have communicated this more clearly…

BenceSzalai commented 1 year ago

Well, never mind. Would have been better to know, but it's fine this way too. I like the way this library is built and I'd like to use it in my app, so I wanted to progress.

It is up to you if you want to push your version and see if we can merge or you can cherry pick my changes, or suggest specific commits I should pick from you and converge that way.

Anyway build setup and tooling should only have little effect on actual runtime code, so it shouldn't create much divergence in behaviour or completely prevent PRs of functional changes.

loilo commented 1 year ago

There we go: I made a v2 branch with my local changes. (Going to merge that back soon – main work should happen in master, as you suggested, but I intend to squash and merge v2 as soon as it's ready for an alpha release, to keep the Git history clean to some extent.) Here's the PR.

So, to come back to the points you made, comparing them against my changes:

  • replaced yarn with npm

I did the same, but reverted to yarn because for some (yet to be investigated) reason, GitHub Actions fails to run npm ci, but has no problems installing dependencies with yarn. πŸ€·β€β™‚οΈ

  • bumped node.js to v18
  • updated lerna to v7
  • updated typescript to 4.9/5.1

Same here.

ts-jest, jest and related libraries as they needed newer versions to work together too

I directly made the switch to Vitest.

  • configured lerna (or more precisely nx) to cache linting, and build [...]

Yes, did the same.

I have noticed that the scripts were configured in a way that the normal test script resulted in eslint running twice

Oops. πŸ˜…

I have changded the scripts to skip the "dry-run" and go straight to --fix

I also simplified the build scripts, but I omitted the --fix. It's not something that should run in CI, and running fixes locally is pretty much trivial (yarn lint:source --fix on a per-package basis).

Strangely the original linting setup was perfectly happy with the state of the repo, however […]

Weird. I did not encounter this because I also updated all used ESLint plugins, so I had a lot of redlining to fix anyway. 😁 Inferring from the violation you saw (esp. around .map() and .reduce()), maybe you accidentally updated some of those plugins as well in your switch from yarn to npm. But that's just a wild guess.

Tokens were reordered in some RegExps (autofix). […] Some line-breaks and boilerplate were added around an array.map() code (autofix). […] Reduce usages are not allowed by unicorn/no-reduce, but I didn't want to introduce any unintentional bahaviour changes so I've just prefixed reduce() usages with // eslint-disable-next-line comments.

Those are all coming from the Unicorn plugin. The plugin intentionally is very opinionated, so I'm quick to just disable individual Unicorn rules that I don't agree with, which kind of makes all of the above obsolete.

The rule unicorn/prevent-abbreviations was not happy about some naming choices (Args vs. Arguments).

Yeah, this made me scratch my head as well, since I explicitly added args to the rule's whitelist (which it respects in other places). Apparently, the whitelist does not work properly with compound words (e.g. ConstructorArgs), so I added eslint-disable-next to those cases.

  • @typescript-eslint/ban-types was not happy about using {} and Function […]

Same here.

I have also replaced the no-redeclare rule with @typescript-eslint/no-redeclare […]

Good pointer, thanks. I only had disabled no-redeclare, but adding the @typescript-eslint equivalent is obviously better.

so the custom run-root solution was replaced with lerna run and some lerna exec usages as well

Yes, I did the same. I actually abolished run-root (and many of the bin/ scripts) altogether.

Since this change requires scripts to be defined on each package's level to make it easier to maintain the scripts section of the packages I've created a utility available with npm run inherit which will copy all scripts defined in package.single.json to each package's package.json.

I did those changes mostly doing search&replace. I like the automated approach, although I'm not sure whether it works everywhere (e.g. I have one-offs in the test scripts of some individual packages, for example @livy/contracts which does not use Vitest).

Testing is a much different animal to lint and build, so right now the tooling for testing is the same.

Yes, this definitely was the largest beast to tame. I put quite some work into it and it's now running on an individual package basis as well with Vitest. The mocking is a bit of a mess (it already was, to be fair, but unfortunately it still is), but it works. I've put a TODO in front of it for clearing that up someday. 😁

Having all the scripts on an individual package basis also has the advantage of way better development experience on individual components, as you can just do yarn vitest in there to get Vitest's hot-reloading testing behavior. Not to mention that test runs naturally are also cached by Nx now.

I've tried few things to utilise Nx for the tests, but the key issue is that if the tests run for each package separately, they cannot generate an overall coverage report.

I thought about this as well, but it came as a pleasant surprise that this actually is a non-problem. Codecov just collects and merges all the individual coverage reports on its own without any further configuration.

I have done the typescript upgrade in two steps: to v4.9 and to v5.1.

I directly went to 5.1 since TypeScript does not attempt to follow semantic versioning anyway (because every release in a type checker naturally contains breaking changes).

That said, I also was surprised that there was not too much to fix here.

All-together none of these changes should affect existing "v1" users in any way.

That's a point where our approaches are diverging significantly. It's kind of sad because there would've certainly been value in having an improved but fully compatible version, but that's where we are.

The reason for breaking changes in my branch is not that I touched any of the APIs (which I don't intend to, unless somebody brings a good argument for doing so), but because I also updated the npm dependencies of all individual components, also across major version bounds.

Almost none of these breaking changes should manifest in Livy's public API, they should only show up at "adapter" points where options are directly passed through to the underlying dependency, e.g. the got options in the @livy/http-handler. But just the fact alone that many of those new major versions dropped support for older Node versions of course is a breaking change in itself.

In the PR, I listed all major dependency updates I did (except some that are strictly internal), so I can come back to this later when writing a migration guide for v2.


Some additional To Dos are mentioned in the PR. Happy to hear your feedback there.

I think we're making progress. πŸ˜„

BenceSzalai commented 1 year ago

Hi! Had to focus on different things for a while, but I still would like to use this lib! :)

15 Rearchitect project in preparation for v2

I've reviewed the latest v2 branch, set up the tooling, got tests to work etc. It is all good.

I assume you don't want any of these changes to land in v1 as you went on directly for the v2 branch.

You have two pending items listed as ToDo in #15.

What else to do?

I don't want to work on the package again without explicit prior coordination, so here's the question: What else is left?

My original goal was to use livy, but I faced the section in the Readme suggesting there are stuff to get sorted first on one hand and the notion that bringing time-zone support might be an unjustified overhead when it comes to logging.

So revisiting these items and what is discussed above few potential areas to progress with are:

@loilo: So long story short, do you have any input for me that I can act on to make things progress?

What I can imagine is to find the points Luxon is used and just get rid of them, replace features with equivalent but assuming that all timestamps in the same timezone as the system and make sure tests pass as well.

What do you think? Thanks!

BenceSzalai commented 1 year ago

I've eventually encountered the issue with top-level await in browser context. This is my solution (for now): https://github.com/BenceSzalai/livy/commit/666d3bc8eb1ff440647ffbb859194a8bf77bd1ae