Skip to content
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

fix: try update @nuxt/scripts implementation #391

Merged
merged 9 commits into from
Mar 9, 2025

Conversation

Yizack
Copy link
Contributor

@Yizack Yizack commented Mar 9, 2025

πŸ”— Linked issue

might fix #390

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Tried to update the module to the latest @nuxt/scripts@0.11.1 implementation and nuxt@3.16.0

Since the component needs to expose reset I assign it inside onLoaded, just in case, I tried using proxy directly but the reset wasn't working

Sorry, something went wrong.

@Yizack Yizack requested a review from danielroe as a code owner March 9, 2025 11:03
@Tkkg1994
Copy link

Tkkg1994 commented Mar 9, 2025

@danielroe
Any idea when we can merge this and get out a new release so we can use the module in nuxt 3.16? Sorry for the hassle...

@danielroe
Copy link
Collaborator

I'll review later today.

have you investigated the failing tests?

@Tkkg1994
Copy link

Tkkg1994 commented Mar 9, 2025

@danielroe yes it's an issue with missing types introduced in Yizack@4b5577c

But @Yizack can answer that why he maybe those changes. Were they needed in the src/runtime/components/NuxtTurnstile.vue ?

@Yizack
Copy link
Contributor Author

Yizack commented Mar 9, 2025 β€’

@Tkkg1994

@danielroe yes it's an issue with missing types introduced in Yizack@4b5577c

But @Yizack can answer that why he maybe those changes. Were they needed in the src/runtime/components/NuxtTurnstile.vue ?

The missing types issue comes from @nuxt/scripts see nuxt/scripts#419

The changes in NuxtTurnstile.vue were needed due to @nuxt/scripts v0.11.0 breaking changes, a new onLoaded function was introduced. read https://github.com/nuxt/scripts/releases/tag/v0.11.0

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Daniel Roe <daniel@roe.dev>
@danielroe
Copy link
Collaborator

@Yizack I've upgraded nuxt/scripts on its own in #389.

You've done a bit here that updates the implementation as well - I'm not sure whether these changes are still necessary, but if they are, would you add some tests that fail on the current implementation? πŸ™

danielroe and others added 3 commits March 9, 2025 16:12
@Yizack
Copy link
Contributor Author

Yizack commented Mar 9, 2025 β€’

Added a test to check if turnstile input elements are rendered after click on Load Turnstile button, this test will fail without the changes in NuxtTurnstile.vue because render instance function isn't directly exposed from useScriptCloudflareTurnstile, instance functions are available inside onLoaded in the latest @nuxt/scripts version

Screenshot 2025-03-09 114857

Any feedback is welcome β€οΈπŸ™

Copy link

codecov bot commented Mar 9, 2025 β€’

Codecov Report

Attention: Patch coverage is 0% with 20 lines in your changes missing coverage. Please review.

Project coverage is 29.34%. Comparing base (dc164cc) to head (01a1dd6).
Report is 163 commits behind head on main.

Files with missing lines Patch % Lines
src/runtime/components/NuxtTurnstile.vue 0.00% 20 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #391       +/-   ##
===========================================
+ Coverage   19.26%   29.34%   +10.07%     
===========================================
  Files          12       12               
  Lines         493      351      -142     
  Branches       23       24        +1     
===========================================
+ Hits           95      103        +8     
+ Misses        398      248      -150     

β˜” View full report in Codecov by Sentry.
πŸ“’ Have feedback on the report? Share it here.

πŸš€ New features to boost your workflow:
  • ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • πŸ“¦ JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@danielroe danielroe merged commit 94deb2c into nuxt-modules:main Mar 9, 2025
5 of 6 checks passed
@danielroe
Copy link
Collaborator

thank you ❀️

dargmuesli-bot pushed a commit that referenced this pull request Mar 9, 2025
## [0.9.15](v0.9.14...v0.9.15) (2025-03-09)

### Bug Fixes

* update to latest `@nuxt/scripts` implementation ([#391](#391)) ([94deb2c](94deb2c))
@dargmuesli-bot
Copy link
Collaborator

πŸŽ‰ This PR is included in version 0.9.15 πŸŽ‰

The release is available on:

Your semantic-release bot πŸ“¦πŸš€

@StirStudios
Copy link

StirStudios commented Mar 10, 2025 β€’

On our end this issue is not fully resolved. We are getting:

[@nuxt/scripts 8:49:50 AM] ERROR Nuxt Scripts requires Unhead >= 2, you are using v1.11.20. Please run nuxi upgrade --clean to upgrade...

"@nuxtjs/turnstile": "^0.9.15",
"nuxt": "^3.16.0",

If we run a fresh install without turnstile the install works without any errors. As soon as we add it back we get the error.

Anyone else?

@Yizack Yizack deleted the update-nuxt-scripts branch March 10, 2025 18:26
@Yizack
Copy link
Contributor Author

Yizack commented Mar 10, 2025

Hello @StirStudios

That issue doesn't seem to be related to this PR, does your project have other nuxt modules installed? it would be great if you can provide a reproduction link so maintainers can investigate further

@StirStudios
Copy link

@Yizack you are totally right, with Nuxt 3.16.0 other changes were made to layers so it was causing an issue here:

extends: [['github:yout_layer', { install: true }]],
So now you do not need to include

{ install: true }

Sorry for any confusion caused!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nuxt 3.16.0 - Rollup failed to resolve import "#nuxt-scripts-utils"
5 participants