Improvement: Generate recaptcha script on demand. closes #175

[felixmosh]

No need to load recaptcha script on the html if one of the views uses it.
Load it on demand when vcRecaptchaService is injected in the first time.
closes #175

[coveralls]

Coverage increased (+0.6%) to 85.246% when pulling b296f81 on felixmosh:generate-link-ondemand into c59c81e on VividCortex:master.

[iambrosi]

@felixmosh First of all, I'd like to thank you for this PR! I like this feature and I think this could partially solve #175

Here are some comments:

• Can you include a test case that proves that this feature works as expected?
• We need a way to tell the service to load the script using a specific language, if configured. The reCAPTCHA script supports a parameter hl, which sets a custom language for the widget.

[felixmosh]

I've wrote that fix in order to solve #175 :]
I will add the tests.
There is no need for specifying the language in the script tag, you can just set with that:
https://github.com/VividCortex/angular-recaptcha/blob/master/src/service.js#L84-L85
It will work, i`ve checked, initially i had the language at the script tag it self, and i started using the setLang method.

[coveralls]

Coverage increased (+3.09%) to 87.705% when pulling 6ecec83 on felixmosh:generate-link-ondemand into c59c81e on VividCortex:master.

[mtrias]

This looks good to me.

Question: If I have my own <script async> that loads recaptcha, and at the time you check for it, my script has not yet completed loading, will I end up loading it twice?

If that's the case, I don't see a good way to fix it. Perhaps we should consider encouraging the dynamic loading (i.e. removing the <script> from the Usage section here) ?

[coveralls]

Coverage increased (+3.09%) to 87.705% when pulling ba287ea on felixmosh:generate-link-ondemand into c59c81e on VividCortex:master.

[coveralls]

Coverage increased (+9.6%) to 94.262% when pulling 7bdc4ac on felixmosh:generate-link-ondemand into c59c81e on VividCortex:master.

[mtrias]

@TheSharpieOne I'd love to hear your thoughts on this one.

[TheSharpieOne]

LGTM, though I'm not a fan of the new test style, but it does have better coverage so I cannot complain.

One small thing that is no big deal if it happens. If a use manually adds the script tag (such as they are currently doing), there is a chance that the script is still being downloaded (since they are async) when this new code is executed which will cause another script tag to be added. To this fix we could look at all of the script tags currently on the page, determine that none are for recaptcha (by looking at the src attr) and then proceed to add our own.

[mtrias]

Ok, yeah, the duplicate script is an issue, and the author tweaked the documentation to remove the recaptcha script from the setup instructions for that reason. I think we should release this as a major version increment, in order to encourage users to remove their script tag. Thoughts?

@felixmosh would you like to implement the check that @TheSharpieOne suggested? It could be addressed as a separate issue otherwise, but would be great if you could.

[TheSharpieOne]

👍 major version bump to avoid ^3.2.1 semvar pulling it in. It will force developers to more/less manually upgrade (unless they are using *, in which case, shame on them). An "upgrade guide" (could be in the github release notes) can be used to inform developers the changes requires them to no longer pull in recaptcha themselves.

[felixmosh]

@mtrias, it should not happen.
The official suggestion will be not to include the script tag and therefore there is no reason to handle something like that.

The current implementation suffers from a more common problem (#175).
I encountered it on production, and that filled my logs with errors, cause usually you will use the plugin only on few of the SPA pages, so the initialization of the callback function will be only on them.

[iambrosi]

@felixmosh Can you update the PR description to indicate that #175 will be closed once this is merged?

[felixmosh]

Done

[mtrias]

ok, fair enough! It seems like we are ready to merge then.

I'll proceed with the merge, release tag, npm publish and release notes.

Thank you all!

[lerignoux]

Having just installed "angular-recaptcha": "4.0.0" I noticed the recaptcha div was not loaded. I still have to include google script reference in my header (and with it it now works). Is there anything I could have done wrong ?
Please tell me if you need any more info, I'm an angular noob so ....

Reproduction:

• browsers: Firefox 50.1.0 & Chrome
• system: Linux
• Angular: 1.4.8
• Traceur 0.0.92

Otherwise I'll open a new issue on this in 4.0.0.

[felixmosh]

@lerignoux whenever u inject the service, it should append the script tag.
I`m suspecting that either you missed the angular module dependency or the directive is not used correctly, can u share code snippet for you usage?