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

feat(element): add support for dynamic module configuration #29

Closed

Conversation

gshokanov
Copy link
Contributor

@gshokanov gshokanov commented Oct 16, 2019

This pull request makes it possible to configure LazyElementsModule with dynamic values known only at runtime, while still allowing for AOT compilation. Implements functionality for #28. The API now supports two signatures:

  • Current signature with static values:
const lazyElementOptions: LazyElementModuleRootOptions = {
    elementConfigs: [
        { tag: 'ba-header', url: 'https://example.com/header-component' }
    ]
};

@NgModule({
    imports: [
        LazyElementsModule.forRoot(lazyElementsConfig)
    ]
})
  • New signature with dynamic factory:
export function lazyElementOptionsFactory(): LazyElementModuleRootOptions {
    return {
        elementConfigs: [
            { tag: 'ba-header', url: resolveUrl('ba-header') }
        ]
    };
}

@NgModule({
    imports: [
        LazyElementsModule.forRoot(lazyElementsOptionsFactory)
    ]
})

Please review and let me know what you think. I'll work on adding documentation later.

- support dynamic config in LazyElementsModule.forRoot
- support dynamic config in LazyElementsModule.forFeature
@codecov-io
Copy link

Codecov Report

Merging #29 into master will increase coverage by 0.19%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #29      +/-   ##
==========================================
+ Coverage   85.86%   86.05%   +0.19%     
==========================================
  Files          30       30              
  Lines         290      294       +4     
  Branches       38       40       +2     
==========================================
+ Hits          249      253       +4     
  Misses         34       34              
  Partials        7        7
Impacted Files Coverage Δ
...ents/src/lib/lazy-elements/lazy-elements.tokens.ts 100% <100%> (ø) ⬆️
...ents/src/lib/lazy-elements/lazy-elements.module.ts 80% <60%> (+3.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6d9a17e...8b4924c. Read the comment docs.

@tomastrajan
Copy link
Member

Hi @MagicCurlyHair !

Sorry for the late response, I think the idea to support that is great! The implementation looks a bit like too much, but I am not sure if it can be reduced to a bit less will explore it and let you know.

@gshokanov
Copy link
Contributor Author

Hey @tomastrajan

I agree that it's far from being minimalistic. I spent quite some time on this issue and this is the only solution that I have come up with that doesn't break anything and seems to be working under all conditions. Maybe there is a better way, but I haven't found any.

@p-wunderlich
Copy link

I like this idea, any news on this? :)

@tomastrajan
Copy link
Member

@pschalk @MagicCurlyHair will try to have a look soon

@tomastrajan
Copy link
Member

tomastrajan commented Jun 9, 2020

@MagicCurlyHair @pschalk I had a look and I think we do not need this solution after conversation with @irekBudzowski as it can be achieved already today without complicating the library further...

The solution is to provide already available LAZY_ELEMENT_CONFIGS token as a multi: true provider with an useFactory function...

@NgModule({
    imports: [LazyElementsModule.forFeature({})],
    providers: [
        {
            provide: LAZY_ELEMENT_CONFIGS,
            useFactory: elementConfigsFactory,
            multi: true
         }
    ]
})
export class SomeFeatureModule {}

export function elementConfigsFactory() {
    // resolve config dynamically
}

The solution then would be to update the docs so it is clear how this can be achieved...

@gshokanov
Copy link
Contributor Author

@tomastrajan great idea, I like it. This PR can be closed then

@tomastrajan
Copy link
Member

So I have added docs which describe how this can be done.

Also, there was a little change to how the LAZY_ELEMENT_CONFIGS are processed internally to make sure that these extra LAZY_ELEMENT_CONFIGS provided direcly using multi: true provider are not lost. (see d0a9842 for more info)

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.

4 participants