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

Multiple Configs are lost #17

Open
adickson311 opened this issue Sep 20, 2019 · 6 comments
Open

Multiple Configs are lost #17

adickson311 opened this issue Sep 20, 2019 · 6 comments

Comments

@adickson311
Copy link

adickson311 commented Sep 20, 2019

If you make multiple calls to LazyElementsModule.forFeature(options), ... you will only get the last configuration and lose references to other configs.

angular-extension bug

In LazyElementModule constructor:

    if (elementConfigsMultiProvider && elementConfigsMultiProvider.length) {
      const lastAddedConfigs =
        elementConfigsMultiProvider[elementConfigsMultiProvider.length - 1];
      lazyElementsLoaderService.addConfigs(lastAddedConfigs);
    }

This should be:

const configs = [].concat.apply([], elementConfigsMultiProvider);
lazyElementsLoaderService.addConfigs(configs);
@adickson311
Copy link
Author

adickson311 commented Sep 20, 2019

Submitted a PR:
#18

@tomastrajan
Copy link
Member

@adickson311 but is it ? the configs are registered in the service right ? so service holds them all, and whenever we call forFeature then we basically append the last one in the service or ?

 addConfigs(newConfigs: ElementConfig[]) {
    newConfigs.forEach(newConfig => {
      const existingConfig = this.getElementConfig(newConfig.tag);
      if (existingConfig) {
        console.warn(
          `${LOG_PREFIX} - ElementConfig for tag '${newConfig.tag}' was previously added, it will not be added multiple times, continue...`
        );
      } else {
        this.configs.push(newConfig);    // <--- this line 
      }
    });
  }

@tomastrajan
Copy link
Member

@adickson311 because the constructors should happen one by one and NOT concurrently, that way we should also append configs to the service one by one and not lose anything ?

@tomastrajan
Copy link
Member

@adickson311 the only way I could reproduce your bug is

@NgModule({
  schemas: [CUSTOM_ELEMENTS_SCHEMA],
  imports: [
    LazyElementsModule.forFeature(options1),
    LazyElementsModule.forFeature(options2),
  ]
})
export class ExampleModule {}

Where only the second option2 will be added, then again, does such an code make sense? It works great with single LazyElementsModule.forFeature(allFeatureOptions) registration...

The usage of last config prevents us from adding single config multiple times because then every new feature would re-add all the previous configs (because of the multi provider) and spam user with warning like this...

@angular-extensions/elements - ElementConfig for tag 'ion-button' was previously added, it will not be added multiple times, continue...

@tomastrajan
Copy link
Member

@adickson311 do you have any other way to reproduce this behavior ?

@felipeplets
Copy link
Contributor

@adickson311 is this still an issue?

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

No branches or pull requests

3 participants