-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(nuxt3): rewrite with nuxt kit #47
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would love to know @pi0 thoughts on my comments
@pi0 @atinux @danielroe I have added all recommendations from you :) |
package.json
Outdated
"access": "public" | ||
}, | ||
"homepage": "https://github.com/nuxt-community/web-vitals-module#readme", | ||
"directories": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What it is used for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was there before, I can remove it :)
configKey: 'webVitals' | ||
}, | ||
defaults: { | ||
provider: 'log', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are already setting default to log
in L44 :)
console.error('[nuxt vitals]', err) // eslint-disable-line no-console | ||
} | ||
|
||
export function logDebug (label, ...args) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you removed log utils? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was used in one place and basically had the same purpose as regular console.log.
Co-authored-by: pooya parsa <pyapar@gmail.com>
Co-authored-by: pooya parsa <pyapar@gmail.com>
Co-authored-by: pooya parsa <pyapar@gmail.com>
Co-authored-by: pooya parsa <pyapar@gmail.com>
Co-authored-by: pooya parsa <pyapar@gmail.com>
I have updated the PR with the recent recommendations. |
Would be nice to have an edge release in order to try it out :) |
I was thinking that you could try to release on a Do you mind resolving the conflicts @Baroshem ? |
Current module should be compatible with Nuxt 3. I'm holding on this PR to properly test for Nuxt 3 and Nuxt 2 compatibility (vercel auto installs this module on deployments) and also check provider resolution changes. |
This is why I was thinking of an edge deployment to test on Vercel for example on one of our Nuxt 3 projects, how to test it otherwise? |
Hi guys, any updates on this one? |
So far the module is working also on Nuxt 3. I would personally vote to have a branch for Nuxt 2 and another one for Nuxt 3 for simplicity. |
@Baroshem I reworked migration to kit with 3.0.0 compatibility in #59 to be without breaking changes and remain nuxt 2 compatible (mainly because vercel is injecting this module automatically). Your PR included some more changes and type improvements that were conflicting. Feel free to rework them as smaller PRs and thanks again for time spent on this migration ❤️ |
In this PR I wanted to introduce some Breaking Changes for the configuration of the module for both Google Analytics and Vercel Analytics.
I wanted to add both these variables as a part of the module config options like this:
Apart from that, no other BC are introduced in the migration and the module should work for all nuxt.js projects due to usage of module-builder.