How to use it in lazy loaded modules?

[mnn]

When I try to import the ReCaptcha module in a lazy loaded modules then the module is loaded several times and I suspect it runs Google installation code every time, because I am getting an error:

ReCAPTCHA placeholder element must be empty

Any ideas how to fix it? Or this package does not support lazy loading?

[achimha]

You got a point here. Lazily loaded modules using angular/cli means that the component gets bundled into each lazily loaded module it is used in and then the external JS library would be loaded multiple times.

We should check whether the library is already in the namespace before we load it.

[achimha]

Can you give this PR a try? https://github.com/xmaestro/angular2-recaptcha/pull/50

[mnn]

Doesn't seem to be working. After more testing the error only happens when entering an application via a lazily loaded route. Maybe it's a race condition? Google script tag has been added to header to be loaded, but not yet, and the lazily loaded route triggers another script tag to be added, because first one hasn't yet been loaded so there is no grecaptcha?

[achimha]

So you load two lazy routes at once?

The proper solution is probably to inject the ReCaptchaService globally. It was actually designed like that for this purpose. Could you give this a try? In your main bootstrap do:

{ provide: ReCaptchaService, useClass: ReCaptchaService }

My PR is probably still the right thing to do.

[mnn]

This hack seems to be working:

            if(window.recaptchaLoaded){
              console.debug('(recaptchaLoaded) recaptcha already installed, skipping');
              this.readySubject.next(true);
              return this.readySubject.asObservable();
            }
            window.recaptchaLoaded = true;

But I want to use the component, not only service. How to do that in a lazy module without importing a recaptcha module?

Edit: I am still learning ng2, so I am not sure how exactly this half-instantiation of a module works. I'll give it a try later.

[achimha]

That doesn't fix the race because it's the same thing.

You have to provide the ReCaptchaService as part of your main bundle, then it will be used by the lazily loaded modules and not created again. That's the proper Angular way. If you could test it, we can change the documentation.

[mnn]

That doesn't fix the race because it's the same thing.

I am confused, the hack did fix the race in my case. Or you mean the PR?

So I can't have only-once instantiated modules across application, but I can have only-once instantiated services, even though they are in a different instance of a module? Not really getting the new ng way...

[achimha]

I doubt your fix "works", due to the nature of a race, it can work and it also can fail. Yes, in your main module, provide the service (which is part of this module). Angular's DI looks if it already has a suitable instance of the service when it creates the component and would use it then.

[mnn]

It doesn't seem like I can import the service, is it exported? I might be doing something wrong :/.

TS2305: Module '".../node_modules/angular2-recaptcha/index"' has no exported member 'ReCaptchaService'.

Despite the error it compiles, but crashes on a same thing (ReCAPTCHA placeholder element must be empty). Any ideas?

[achimha]

The issue is more complex than I thought.

Every (lazily loaded) module using the ReCaptacha component has to import it in its module declaration. This means Angular will create a new instance of the ReCaptchaService and there is no way around that.

Protecting against the external JS loading twice is a hack and an ugly one. The ReCaptchaService should be a singleton throughout the application. I believe the only way to achieve that is to make a module of its own for the service and require users of the component to import that module once in the app module. This would be a breaking change.

[achimha]

I have updated my PR and put ReCaptchaService into its own ReCaptchaServiceModule. The logic to check for an already loaded script was removed.

I think this is the only proper way to deal with this issue. Unfortunately it marks an incompatible change because users have to import an additional module now. We should therefore increase the major version number and document the change.

What do you think?

[mnn]

It doesn't seem to be working any different*, but I might have done something wrong.

I have added { provide: ReCaptchaService, useClass: ReCaptchaService } to AppModule and I am using just plain ReCaptchaModule in imports in my shared module (imported by every lazily-loaded module).

*: My main app component (eagerly loaded) contains one reCaptcha component. When entering an app via a page without lazily loaded component (basically a sub-page) containing reCaptcha it is fine (no errors, eagerly loaded reCap. component and later lazily loaded [after navigating to different pages] reCaptchas are fine). When entering an app via a page with lazily loaded component containing reCaptcha I get the error and the first (eagerly loaded) reCaptcha component is not rendered. But only that one - when navigating through the app other reCaptchas in lazily loaded modules are rendered and working.

[xmaestro]

Let me know if this works and i will do the merge and create the release.

[achimha]

If you use my PR, you don't provide ReCaptchaService but you import ReCaptchaServiceModule in your root module and continue to import ReCaptchaModule in all your (lazily loaded) modules.

This ensures that there is only one ReCaptchaService.

[xmaestro]

Could you please update the README file as well and create a new PR with all the changes?

[mnn]

@achimha Great job! Just tested it now again and your current PR branch is working nicely in all cases :smile:.

[achimha]

I want to do one more test to see if I can avoid this incompatible change.

[achimha]

OK, I found a much better way that is fully backwards compatible. The method is described here: https://github.com/angular/angular/issues/13854

The component now uses a factory to create the service if it doesn't already exist. This means only the first component that is instantiated will create a service instance and subsequent components will use the existing.

Please give it another test on your end before it can be merged.

[xmaestro]

Great! I'll try it out and if it works i'll merge it. Thanks!

[achimha]

There seems to be a problem with AoT although I don't understand it.

[achimha]

I think you can go ahead with the merge, seems to be something about my build system.

[xmaestro]

Created a new release. Please reopen if it does not work.