sainsburys-tech / next-logger

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

suggestion: `createRequire` #2

Closed BarryThePenguin closed 2 years ago

BarryThePenguin commented 3 years ago

Was looking to see how the logger is replaced and saw these two lines..

https://github.com/atkinchris/next-logger/blob/cde1af765c49666d6a36cb5506db00d3d2353a43/lib/patches/next.js#L1-L4

You might be able to replace them with..

const { createRequire } = require('module');
const nextPath = require.resolve('next')
const nextRequire = createRequire(nextPath)

// This is relative to 'next/dist/server/next.js'
const nextLogger = nextRequire('../build/output/log')

..as something that uses the node modele API to lookup the file.

I wouldn't say this approach is better.. just less dynamic? And no longer needs to use process.cwd() to find file.

According to their documentation, this should work on Node v12.2.0 and above

atkinchris commented 3 years ago

Thanks for this - I hadn't seen this before in the Node API. I'll take look!

atkinchris commented 2 years ago

I'm wondering if this could be simplified even further to just:

const nextLogger = require('next/dist/build/output/log')

It should take away the requirement for node_modules/next to be at the root, as you've done by creating a scoped require.

BarryThePenguin commented 2 years ago

Maybe! I thought there may have been a reason for the original implementation.

The original module may may have not been in the dist directory, so stepping out of that directory may have been necessarily

atkinchris commented 2 years ago

I'm struggling to remember why I did it like this before. 😅

The current implementation breaks down if you have nested node_modules, like in a monorepo without hoisted tooling.

// /Users/username/my-next-project/node_modules/next/dist/build/output/log
require('path').join(process.cwd(), 'node_modules', 'next/dist/build/output/log') 

// This shouldn't work natively if it's nested.
// /Users/username/my-next-project/modules/my-next-server/node_modules/next/dist/build/output/log
atkinchris commented 2 years ago

I've got a branch and tag with this on - with the simplified require and variants to the existing test suite to prove it out in root and nested directories.

https://github.com/atkinchris/next-logger/tree/simplified-requires

It's on a tag, if you want to try it out with your application:

npm install "https://github.com/atkinchris/next-logger.git#v2.2.0-beta.0"

@gustavosf - I've seen you're doing the same thing on your fork (https://github.com/gustavosf/next-logger/commit/073d4d11016c8d001123b7b9bc1e673189390029).

Does this beta version work for you?

gustavosf commented 2 years ago

nice, @atkinchris. I tried it out here on a project we have and this solution works. In my specific case, along with that, I'll also need a way to re-format the log (to stackdriver's json format, specifically), so I was checking some alternatives

atkinchris commented 2 years ago

I've merged this into main after independent testing, and it's released in v2.2.0. If you experience any issues, please reopen this!

https://github.com/atkinchris/next-logger/releases/tag/v2.2.0