thomvaill / log4brains

✍️ Log and publish your architecture decisions (ADR)
Apache License 2.0
1.13k stars 95 forks source link

fix: nextjs incrementalCache usage fixed #79

Closed indatawetrust closed 2 years ago

indatawetrust commented 2 years ago

hi, with nextjs 10.2 version, the method of applying the incretalCache method has changed. current usage fixed

https://github.com/vercel/next.js/discussions/22384#discussioncomment-844774

indatawetrust commented 2 years ago

hi @thomvaill , do you have any comment for this pr?

zenwork commented 2 years ago

Is there a workaround to use this fix from this branch until the PR is merged and a new version is published? I am able clone and build but then I do not know how I should install my local build to use it

indatawetrust commented 2 years ago

@zenwork you can use patch-package

florian-hehlen commented 2 years ago

If you are going to use patch-package please note that I got it working by using log4brains as a dev dependency rather than as a global dep. it also means that i had to wrap calls to log4brains into package.json scripts. But the good news is everything is working... even the github action to deploy to pages.

mathiasmaerker commented 2 years ago

@florian-hehlen How did you get it to work? I installed it localy as well and started via npm scripts but I get the same error. patching via patch-package didn't work either for me.. Any other suggestions?

zenwork commented 2 years ago

@mathiasmaerker , I did the following:

from here on in I need to use npm start and npm run new instead of global commands

also make sure to uninstall your global installation

mathiasmaerker commented 2 years ago

@zenwork thanks for your help but unfortunately I tried that with no success

Just to be clear:

but the result always is the same: ⁉️ Not creating patch file for package 'log4brains/@log4brains' ⁉️ There don't appear to be any changes.

so what am I doing wrong?

Thanks in advance

zenwork commented 2 years ago

it should be npx patch-package @log4brains/web

try that

mathiasmaerker commented 2 years ago

@zenwork I tried that to, but because @log4brains is not part of the package.json you get the error node_modules/@log4brains/web/package.json

zenwork commented 2 years ago

I don't know why you would have this issue. I did not have any trouble

mathiasmaerker commented 2 years ago

@zenwork Well, no clue either :( but thanks a lot for your help and effort 🥇

Sobi-WanKenobi commented 2 years ago

@thomvaill , hopefully you are feeling better and work is lightening up a bit. Any chance you will you be able to merge this PR soon? Thanks for the work here -- really like this project a lot.

thomvaill commented 2 years ago

Hey thank you so much @indatawetrust, you rock! This makes me think that we definitely need to implement https://github.com/thomvaill/log4brains/issues/77 ; that's why I updated your branch before merging by pinning the version of Nextjs to avoid this kind of issue in the future. But we definitely need to do this for all the depts. I will try to work on this next week. Also, this issue was not caught by the scheduled non-regression tests because we only test the build and not the preview. Contributions are welcome for this, because I don't see an easy way to test this ;)

Again sorry for the delay... It's a bit difficult for me to find some time to work on the project, but hopefully your comments and contributions are really motivating, thank you for this!

thomvaill commented 2 years ago

Apparently there is an issue when upgrading from 1.0.0-beta.12 to 1.0.0-beta.13: the changes are not taken into account. Do you confirm? Let's discuss this on https://github.com/thomvaill/log4brains/issues/80