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

IgxSelectionAPIService should be provided on a component level #13938

Closed
teodosiah opened this issue Feb 20, 2024 · 3 comments
Closed

IgxSelectionAPIService should be provided on a component level #13938

teodosiah opened this issue Feb 20, 2024 · 3 comments
Assignees
Labels
📈 enhancement combo select Select component simple-combo ❌ status: rejected Status for feature-requests that we can't or won't implement.

Comments

@teodosiah
Copy link
Contributor

Related to #13923
IgxSelectionAPIService should be provided on a component level instead of on a root level
image

@teodosiah teodosiah self-assigned this Feb 20, 2024
@teodosiah teodosiah added 🛠️ status: in-development Issues and PRs with active development on them and removed 🆕 status: new labels Feb 20, 2024
@radomirchev radomirchev added ❌ status: rejected Status for feature-requests that we can't or won't implement. and removed 🛠️ status: in-development Issues and PRs with active development on them labels Feb 21, 2024
@damyanpetev
Copy link
Member

Clarification on the rejected status - after discussing today again, we're actually of the opinion this change might not be desirable. The root-provided service should only be included when actually needed and tree-shaken otherwise and, once the related leak was resolved, its overhead (empty map more/less) is quite minimal. On the other hand, providing it per component (sandboxing it) will create instance per component and there's a real concern with repeated/templated Selects/Combos for example and more dropdowns around the view UI we can end up with a hefty number of instances, which could be a worse tradeoff.

@kdinev kdinev closed this as completed Feb 23, 2024
@damyanpetev damyanpetev closed this as not planned Won't fix, can't repro, duplicate, stale Feb 23, 2024
@pmoleri
Copy link

pmoleri commented Feb 26, 2024

@damyanpetev somehow 1 service instance per component seems more expensive than 2-Map entries per-component? Can't be 100% sure, but my hunch is that the opposite is true.
IMO, root service means more complexity and more chances for mistakes (as already proven). And only worth it if there's a use case for it.

As a side note, I remember a conference from Ryan Dahl mentioning that he regretted not having Promises for node async APIs because his initial thought was that it meant an extra object per API call.

@pmoleri
Copy link

pmoleri commented Feb 26, 2024

Also, it uses these ids which if I'm not mistaken their sole purpose is to be able to track the instances in the root service.
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📈 enhancement combo select Select component simple-combo ❌ status: rejected Status for feature-requests that we can't or won't implement.
Projects
None yet
Development

No branches or pull requests

5 participants