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

Register components lazily #136

Merged
merged 2 commits into from
Sep 20, 2024
Merged

Register components lazily #136

merged 2 commits into from
Sep 20, 2024

Conversation

mabdullahsari
Copy link
Contributor

Hi! 👋

I was trying to test a contact form page until a pesky problem reared its head:

Illuminate\Contracts\Container\BindingResolutionException: Unresolvable dependency resolving [Parameter #1 [ <required> string $manifestPath ]] in class BladeUI\Icons\IconsManifest

Further investigation lead me to laravel-honeypot as the package seems to register its services eagerly. I do recognize not everyone is making use of application siloing (registering only the necessary service providers at the last possible moment), but this change would also benefit every application by becoming a lazy service registrar. callAfterResolving is a method that has been in the framework since v5.6, so it won't break BC. (Please see this article for more context).

I have also removed the $this return in the last method in the chain of calls, as it was unused.

Thank you! 🙏

@riasvdv riasvdv merged commit 5772783 into spatie:main Sep 20, 2024
18 checks passed
@riasvdv
Copy link
Member

riasvdv commented Sep 20, 2024

Thanks!

@dwightwatson
Copy link
Contributor

I believe this patch may have caused issues for me - my app now fails to run php artisan optimize after updating from 4.5.2 to 4.5.3.

InvalidArgumentException
 
/vendor/laravel/framework/src/Illuminate/View/Compilers/ComponentTagCompiler.php in Illuminate\View\Compilers\ComponentTagCompiler::componentClass
Unable to locate a class or view for component [icon-circle-check].

The cause of the error is Blade UI kit trying to find trying to find an icon, but because this change appears to interact with the Blade compiler I suspect it is the culprit.

I'm having a little trouble narrowing down why, as so far it's only reproducible on my servers.

Things of note:

  • Why are the callback functions static - do they need to be/is this causing trouble
  • Should the class name BladeCompiler::class be passed to the method instead of the internal blade.compiler string?

@dwightwatson
Copy link
Contributor

I believe I've narrowed this down to the view composer replacement. When I revert that back to using the facade the problem goes away.

Would reverting the view composer back affect the original problem this PR set out to solve?

@mabdullahsari
Copy link
Contributor Author

Hey @dwightwatson

I'm so sorry it caused troubles on your end! Normally, this change should not have affected anyone because the façade calls are doing the exact same thing as in the lazy registrations. It's just that the registrations now only happen right before the objects are requested through the container.

Why are the callback functions static - do they need to be/is this causing trouble

They don't have to be static; it's just a slight performance optimization as the $this context is not needed within the closures.

Should the class name BladeCompiler::class be passed to the method instead of the internal blade.compiler string?

The blade.compiler symbol eventually resolves to BladeCompiler which is the service behind it.

Would reverting the view composer back affect the original problem this PR set out to solve?

Yes, unfortunately it would.


Does this error messages arise when the view:cache part is taking place? If you can provide me a bit more information, I'll try looking into it and see what's different because in theory, there should be no difference... 😔

@dwightwatson
Copy link
Contributor

What's super odd about this is that it sounds awfully similar to a bug I experienced earlier inside Laravel, though it was thought to have been solved. However others have since popped up having the same problem.

The tl;dr being that using callAfterResolving appears to replace the instance and so bindings from Blade UI kit were lost.

You mention that the facade does the exact same thing but clearly that isn't the case. Not really sure what the solution is here, I've had to lock to the previous version but would prefer to find a resolution.

@dwightwatson
Copy link
Contributor

Apologies - forgot to reply to your last bit. The server in question is Heroku so actually getting the logs out of it when this occurs is pretty impossible. All I've got is the stack trace out of Sentry.

InvalidArgumentException
Unable to locate a class or view for component [icon-chevron-down].

/vendor/laravel/framework/src/Illuminate/View/Compilers/ComponentTagCompiler.php in Illuminate\View\Compilers\ComponentTagCompiler::componentClass at line 311

        if (Str::startsWith($component, 'mail::')) {
            return $component;
        }
        throw new InvalidArgumentException(
            "Unable to locate a class or view for component [{$component}]."
        );
    }
    /**

I'll try to keep playing around to see if I can find a better way.

@mabdullahsari
Copy link
Contributor Author

Hey man! I haven't forgotten about this. I'll try taking a look myself tomorrow. Have you been able to find anything useful in the meantime? Thanks!

@dwightwatson
Copy link
Contributor

All good - no, I haven't come up with anything of substance. And because the issue is only presenting itself on Heroku dynos the feedback loop doesn't make it easy.

I think there's just some gnarly weird bug under the hood that has some relationship between the optimize command sharing an app instance and the container bindings. And clearly whatever triggers it isn't widespread enough that a lot of people have run into it.

I wrote a note about this on the laravel/framework issue to see if the callAfterResolving might be a clue, see if anyone else notices a similarity there. Unless you're in the mood to dig into the internals I'm not sure there's more that can be done here without a simpler reproduction to test agains.

Otherwise I think I may drop using the optimize command and call all the cache commands separately.

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

Successfully merging this pull request may close these issues.

3 participants