-
-
Notifications
You must be signed in to change notification settings - Fork 208
Setting up strategies. #186
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
Conversation
Further discussionUse cases
Required functionality
TokensSeparate tokens for default, defaultOutOfVP, and available strategies. Confiuration
Consuming strategies
Runtime change
Registering new strategies
Requirements satisfaction
Cons
ServiceService that will serve as a "store" for strategies config. Configuration
Consuming strategies
Runtime change
Registering new strategies
Requirements satisfaction
Cons
Questions
Implementation suggestionTBD |
No that's not a good idea. Exposed packaged should not depend on each other. The only thing I could think of is that
It would be super confusing imho
e.g. |
@kajetansw any ideas? |
Also thought about it. I would say we make it stable as is, restructure it, and then talk again. |
One solution I can think of is for the user to pass custom strategy names when initializing/declaring the // 1
const service = new StrategySetupService<'test'>();
const s = service.select().pipe(
filter(s => s.currentStrategy === 'test')
// ^^^^
// OK, TypeScript won't let us use other values than
// "global", "native", "local", "detach", "noop" (default strategies) and "test"
);
// 2
const service = new StrategySetupService<'test'>();
const s = service.select().pipe(
filter(s => s.currentStrategy === 'other')
// ^^^^
// ERROR, "other" doesn't match values above
);
// 3
const service = new StrategySetupService();
const s = service.select().pipe(
filter(s => s.currentStrategy === 'other')
// ^^^^
// ERROR, "other" doesn't match default strategy names
);I can try and write some example implementation on a separate branch - some prototype to try out. |
|
@kajetansw Thanks for your answer!! Examples tp poke around sounds good to me! |
|
@kajetansw Thank you 👍 |
|
I did some magic on typings for Play with it please and feed me back on what you think, if you find any caveats or corner cases and if it's of any use altogether 😉 Assumptions and trade-offs of the implementationFor it to work as it is I needed to change two things:
Without the unified constructor across all strategy definitions I wasn't able to create working types for custom strategies. I'm not saying it's impossible though (I don't know that much) but would take some time to investigate further. What do you think about those assumptions? Are they something worth keeping, or should I keep digging? Comments along the wayIf I may, I'd like to propose two things:
|
|
About your questions @Karnaukhov-kh: I think the sole pros/cons ratio showed us that solution with a service is cooler 👍
I personally don't think it's THAT bloated. The service seems to be doing what it is supposed to do - to store and change strategies. Unless of course the implementation is not over and it would be 3 times larger than it is now 😉
How do you propose to split it? Like I wrote above, I don't think the |
|
@kajetansw Very cool, thank you! I'll dig into it more and ping you back.
This is a very nice naming 👍.
The main idea was to store strategies separately. Because we want to keep all registered strategies global and this setup service might be provided on multiple levels. Actually here's a new question 💡. If we need them globally do we need to register new strategies at run-time? We can go with |
|
That's a good point... I personally can't think of any use case of registering custom strategies at runtime. (Doesn't mean that there are none 😉) Registering strategies on module level may also have a very few use cases. I think it may be a good approach to follow YAGNI and start with what you said - to enable users to register strategies globally, only once, with |
|
Hi again @kajetansw! As for me, your idea works 👍. Thanks a lot! Would you like to create a PR from your fork into this PR? (otherwise I will just copy-paste 😁)
Maybe instead we can consider adding config to NoopStrategy function?
Also, think with should change |
|
Sure thing! I'll create a PR within the next hour or so.
I did just that and everything seems fine, so I'll include it in the PR.
Nice catch, I'll also do that :) A new question arose in the meantime: is there some reason why files with only type/interface declarations don't have EDIT: Here you go: #196 😉 |
…rategy names with custom ones
…up-service Types for extending default strategies with custom ones
|
Merged.
Haven't investigated this topic. But will check, thank you 👍. |
… strategies-setup # Conflicts: # apps/template-demo/src/app/app-component/app.component.html # apps/template-demo/src/app/app.routes.ts
|
Kirill, could you put the strategy service as poc in the experiments folder? We need it to poc the rxLet with embeddedView rendering. |
Scope
This PR will be focusing on setting up strategies on global/feature/component levels and passing these settings to existing directives.
All information below is a very early work in progress and just topics to discuss.
InjectionTokens approach
3 tokens introduced.
RX_AVAILABLE_STRATEGIESwith all available strategies.RX_ANGULAR_DEFAULT_STRATEGYdefault strategy for elements in the viewport ('local').RX_ANGULAR_DEFAULT_OUT_OF_VIEWPORT_STRATEGYdefault strategy for elements out of viewport ('noop').Changing default strategies.
Adding a new strategy.
Consuming strategies
@Inject().Service approach
StrategySetupService introduced. Implemented with RxState.
State interface
API
setStrategy. Accepts strategy name.setOutOfViewportStrategy. Accepts strategy name.addStrategy. Accepts request object.This part need to return full strategy function instead of name. Or we should split responsibilities between RxRenderer & SetupService :)
Other
Some typing introduced for strategy names, constructor functions, etc.