-
Notifications
You must be signed in to change notification settings - Fork 51
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
[Help needed] Nuxt 3 support #119
Conversation
Also added docs to the module config parameters
Hi, When I pull your code the only thing that get pulled is I've tried with:
also pulled the original but all with the same result Any idea how to get all files? Thank you! |
Nothing appears because the module usually is built and uploaded to npm so the build files are ignored by Git. I built the module and temporarily included the build files, if you install it again it should work.
Update: TypeScript autocomplete works with the latest commit. |
Would love to see this merged in as a |
This module will need an update to work with Nuxt 3 rc-9 and above. Currently in 3.0.0-rc.9 it gives the following error:
This is due to nuxt/framework#7116 |
Thanks for the solution, the module has been updated to RC9. |
+1 for releasing this under a |
Thank you for your PR! 🙏 |
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.
Left some remarks
dist/module.cjs
Outdated
@@ -0,0 +1,5 @@ | |||
module.exports = function(...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.
Any reason why you push the dist
folder Ideally this will be auto-generated and only available on NPM.
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.
You're correct, that was intentional because as requested here, people were trying to install it from GitHub. It wasn't included when I opened this PR, but 5 months passed and no one from this repository showed up so I temporarily included it.
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.
Ahh, got it 👍🏻
Yes, that makes sense - alternatively feel free to publish a temp package so people can use that while the PR is pending 😋
@@ -1,12 +0,0 @@ | |||
const fetch = require('node-fetch') |
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 removing the generation script here?
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.
Honestly I have no idea, maybe it wasn't needed to make the module work so I forgot about it, I've restored it.
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.
The idea was supposedly to have an easy way to generate the browser regexes so it isn't necessary to do it manually 😋
src/module.ts
Outdated
name: 'device-module', | ||
configKey: 'device', | ||
compatibility: { | ||
nuxt: '^3.0.0-rc.11 || ^2.16.0', |
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.
If it is compatible with previous RCs, I'd suggest to use a lower version (lowest compatible one)
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.
Lowered to RC5.
}, | ||
setup (options, nuxt) { | ||
if (options.enabled) { | ||
nuxt.options.runtimeConfig.public.device = defu(nuxt.options.runtimeConfig.public.device, { |
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.
Not necessary to merge options manually
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.
Thanks I removed it, I put it because I saw it in other Nuxt modules, as I said in the PR description this is my first module so I don't have the experience and the guides aren't much beginner friendly.
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.
More than understandable. Does/did the module author guide help you?
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 kinda did, maybe 5 months ago something was missing or wasn't explained clearly, I've checked it again and it seems fine.
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.
I was trying to implement the tests, and noticed that the removal of the merge options prevents the plugin to access the module options.
Inside ./src/runtime/plugin.ts
accessing useRuntimeConfig().public
returns an empty object.
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.
Ohh, sorry. My bad! Didn't read correctly and thought you merge the options themself, not the runtimeConfig
.
So, ignore what I said and feel free to reintroduce the changes 🙈
cc @danielroe, would really like to have your opinion on the PR too |
Regarding tests: I suppose that Nuxt's test-utils will come in handy - but I am not sure how easy it will be to "fake" the user agent for tests 🙈 |
…sion to RC5" This reverts commit 84b575e.
I found a way to recreate the old tests with Nuxt test utils, it isn't elegant but seems to be working. |
LGTM! cc @dotneet |
@Redemption198 Thank you for your great work! |
@Redemption198 3.0.0 has been released to npm! |
Nice! |
This is my first module so probably something isn't correct, I ported the device module to Nuxt 3 by rewriting it from scratch using the old code and the module starter from CLI, it should behave like the old one.
What's missing:
TestsPublication workflowNuxt 2/Bridge support(Marked as supported, needs to be tested)(^ I don't have experience with these ^)