search

nuxt-modules / turnstile

🔥 Cloudflare Turnstile integration for Nuxt
https://cloudflare.com/products/turnstile
MIT License
215 stars 17 forks source link

fix: remove turnstile widget on before unmount #268

Closed huang-julien closed 10 months ago

huang-julien commented 10 months ago

Hi :wave:

this resolve #267 by removing the turnstile widget. It prevents a callback loop.

codecov[bot] commented 10 months ago

Codecov Report

Merging #268 (11dc742) into main (02b70d6) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #268   +/-   ##
=======================================
  Coverage   82.60%   82.60%           
=======================================
  Files           2        2           
  Lines         115      115           
  Branches       13       13           
=======================================
  Hits           95       95           
  Misses         20       20

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

dargmuesli commented 10 months ago

@huang-julien I've made the test expect to fail waiting for a console log. I don't know of any other way either to verify no error is thrown in Cloudflare's Turnstile library. Also, the remove function uses the same el the reset function uses already instead of the id.

huang-julien commented 10 months ago

Not sure about it, the reset interval of <NuxtTurnstile> is set to 2500 ms and is not configurable. Should we make it configurable ?

danielroe commented 10 months ago

yes, that seems good to me. :+1: the 2500 was chosen to be sufficiently underneath the expiry of the token but I don't see a reason it shouldn't be configurable

huang-julien commented 10 months ago

Also, the remove function uses the same el the reset function uses already instead of the id.

https://developers.cloudflare.com/turnstile/get-started/client-side-rendering/#remove-a-widget

They should update their docs 🤔 even the reset function is mentioned with only id: string signature. Do you want to use the element instead of id returned by render ?

edit: oh their docs seems to be open-source (and using vue ;) )

dargmuesli commented 10 months ago

I've changed the code back to id usage instead.

dargmuesli commented 10 months ago

Thank you very much for the PR, @huang-julien! :heart: @danielroe I think it's time for a new release :partying_face: