search

sainsburys-tech / next-logger

JSON logging patcher for Next.js
MIT License
144 stars 14 forks source link

Re-commit major-ts-based-refactoring #18

Closed devinrhode2 closed 6 months ago

devinrhode2 commented 2 years ago

re-committed all the changes in major-ts-based-refactoring, to be a bit more condensed

@atkinchris if you need this broken up further into separate PRs lmk

I haven't restored the prettier settings yet as I don't know if any of these changes are of real interest to you

The changes are generally sorted from least opinionated in the beginning, to most opinionated at the end (prettier settings being an exception)

I think the history in https://github.com/atkinchris/next-logger/pull/15 is actually better because it doesn't try to hide interesting parts of the journey, but the history here is more useful in some ways because there are fewer commits.

atkinchris commented 2 years ago

I haven't changed the prettier settings yet as I don't know if any of these changes are of real interest to you Prettier changes have slipped into this PR, for example the test files which have been completely reformatted.

As I commented on #15, I'm still concerned that there's significant code change in this PR, bringing it over to JSDoc & types - but there's also functional changes, in things like createNextShuffler or method maps. These would need to be introduced in separate changes, so we can discuss and evaluate the value in each change - and limit the risk of bugs creeping in. If these fix bugs or solve specific problems, then taking each one in turn will give it the safety and space it needs.

atkinchris commented 6 months ago

You've done an enormous amount of work here; thank you for that. However, it makes significant changes in addition to adding TypeScript, as outlined in comments on this PR.

After nearly two years, we're not able to reconcile those changes, so I'm closing this to make it clear that this isn't a change users should expect to see merged in this state.