Refactor module to include a configuration when calling #forRoot

[HunteRoi]

Please check if the PR fulfills these requirements

• The commit messages follow these
  guidelines
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (check one with "x")

• Bugfix
• Feature
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no api changes)
• Build related changes
• CI related changes
• Other. Please describe:

Fixes #62

What is the current behavior? (You can link to an open issue here, add screenshots…)
You can't pass any global configuration to NgxSkeletonLoaderModule#forRoot.

What is the new behavior?
You can pass a global configuration to NgxSkeletonLoaderModule#forRoot that allows you to avoid repeative set of attributes on the ngx-skeleton-loader components.

Does this PR introduce a breaking change? (check one with "x")

• Yes
• No

Other information (if applicable):
I tried to avoid any breaking change when working on this feature.

Also, I need help with the tests as I have litteraly never experienced writing them!

---

@willmendesneto

[HunteRoi]

@willmendesneto Follow-up to your comment...

I understand this implementation is repetitive, harder to maintain and not completed (it misses the tests part so far). I need to keep working on it.

Currently, the config parameter is a class because it cannot be a type:

  static forRoot(config: NgxSkeletonLoaderConfig = defaultConfig): ModuleWithProviders<NgxSkeletonLoaderModule> {
    return {
      ngModule: NgxSkeletonLoaderModule,
      providers: [{ 
        provide: NgxSkeletonLoaderConfig, // <-- this cannot be a type
        useValue: config
      }],
    };
  }

I haven't figured out yet how to implement the feature as you detailed in your comment. If you have any advice, I'd be glad to hear from you!

---

EDIT: see new changes for latest update

[willmendesneto]

I added some comments to have that working as expected by fixing the pipeline commands (for some reason this pull request hasn't triggered CircleCI pipeline, I'm looking at that) and simplifying the code.

Great pull request, mate!

[HunteRoi]

Thanks for your review! I will start working on resolving each later today (I'm from Belgium, it's 3 pm here!).

I added some comments to have that working as expected by fixing the pipeline commands (for some reason this pull request hasn't triggered CircleCI pipeline, I'm looking at that) and simplifying the code.

It is because it is a draft PR. This might be useful in your research: https://ideas.circleci.com/cloud-feature-requests/p/add-support-for-github-draft-prs

Also, I look forward to your comments about how to deal with the component, module & service tests for this PR!

[willmendesneto]

It is because it is a draft PR

TIL! Good to see that CircleCI/GH is getting smart on that 😄

In regards to test, this is definitely a good question! The way I can see tests is that we can cover that by using some real scenarios on modules and components which are already there. E.G.

Module

The test is covering the changes on module + service + component integration

describe('NgxSkeletonLoaderModule.forRoot() method', () => {
  // tslint:disable-next-line: no-any
  let fixture: any;

  beforeEach(
    waitForAsync(() => {
      spyOn(console, 'error');
      spyOn(console, 'log');
      spyOn(console, 'warn');
      spyOn(console, 'info');
      fixture = TestBed.configureTestingModule({
        imports: [NgxSkeletonLoaderModule.forRoot({ appearance: 'circle' })],
        declarations: [ContainerComponent],
        providers: [{ provide: PLATFORM_ID, useValue: 'browser' }],
      }).createComponent(ContainerComponent);
      fixture.detectChanges();
    }),
  );
  
  // This test is now checking if the config was received as expected from the component
  it('should render the component using given forRoot() config', () => {
    expect(fixture.nativeElement.querySelectorAll('.skeletons-defaults .loader.circle').length).toBe(1);
  });
});

Service

The tests for the service itself will be simple since it was simplified to cover if:

• It's combining the configuration with the one received via forRoot (in that case, constructor)
• It's using the default one, in case of not receiving anything
• It's using all the received configuration, in case of all the config was changed via forRoot (in that case, constructor)

// Straight Jasmine testing without Angular's testing support
describe('NgxSkeletonLoaderConfigService', () => {
  it('should use the default config if service is not receiving any specific config', () => {
    const service = new NgxSkeletonLoaderConfigService();
    expect(service.config).toEqual({ /* default object values */});
  });

  it('should combine configuration with the one received via constructor', () => {
    const config = {appearance: 'circle' };
    const service = new NgxSkeletonLoaderConfigService(config);
    expect(service.config).toEqual({ .../* default object values + { } */, ...config});
  });

  it('should override all defaults with if receives configuration via constructor', () => {
    const config = {
      appearance: 'circle',
      animation: 'pulse',
      loadingText: 'Content is loading, please wait ...',
      count: 2,
      theme: {background: 'red'}
    };
    const service = new NgxSkeletonLoaderConfigService(config);
    expect(service.config).toEqual(config);
  });
});

Component

The tests for the component is already covered on the other tests, but it would be good if we have just a test - nice to have in that case since it's covered already. So I'll leave it up to you since it's a nice to have

[HunteRoi]

I don't feel confident enough with writing the tests. Anyone willing to participate on that part?

[dev054]

Btw, thanks @HunteRoi for this initiative. I'm excited to refactor my app with these defaults 🙏

[willmendesneto]

@HunteRoi Removing the draft option from this pull request, so we can have the pipeline validation here for tests, SSR, bundlesize, etc, and address these points.

Also, thanks @dev054 for jumping in and review the changes!

[HunteRoi]

After reading @dev054's comments and placing myself in the point of view of a developer accessing the component's properties (like via @ViewChild() decorator), I decided to mark all attributes as NonNullable<T>, set the component's parameter as optional although there is the @Optional() decorator and also used Partial<T> instead of redundantly marking all fields as optionals.

[HunteRoi]

What about simply adding this test in the module (as we don't have the service to test anymore) ? @willmendesneto

describe('NgxSkeletonLoaderModule.forRoot({}) method', () => {
  // tslint:disable-next-line: no-any
  let fixture: any;

  beforeEach(
    waitForAsync(() => {
      spyOn(console, 'error');
      spyOn(console, 'log');
      spyOn(console, 'warn');
      spyOn(console, 'info');
      fixture = TestBed.configureTestingModule({
        imports: [NgxSkeletonLoaderModule.forRoot({ appearance: 'circle' })],
        declarations: [ContainerComponent],
        providers: [{ provide: PLATFORM_ID, useValue: 'browser' }],
      }).createComponent(ContainerComponent);
      fixture.detectChanges();
    }),
  );

  it('should render the component using given forRoot() config', () => {
    expect(fixture.nativeElement.querySelectorAll('.skeletons-defaults .loader.circle').length).toBe(1);
  });

  it('should NOT call console.error() method', () => {
    expect(console.error).toHaveBeenCalledTimes(0);
  });

  it('should NOT call console.log() method', () => {
    expect(console.log).toHaveBeenCalledTimes(0);
  });

  it('should NOT call console.warn() method', () => {
    expect(console.warn).toHaveBeenCalledTimes(0);
  });

  it('should NOT call console.info() method', () => {
    // tslint:disable-next-line: no-console
    expect(console.info).toHaveBeenCalledTimes(0);
  });
});

[willmendesneto]

What about simply adding this test in the module (as we don't have the service to test anymore) ? @willmendesneto

describe('NgxSkeletonLoaderModule.forRoot({}) method', () => {
  // tslint:disable-next-line: no-any
  let fixture: any;

  beforeEach(
    waitForAsync(() => {
      spyOn(console, 'error');
      spyOn(console, 'log');
      spyOn(console, 'warn');
      spyOn(console, 'info');
      fixture = TestBed.configureTestingModule({
        imports: [NgxSkeletonLoaderModule.forRoot({ appearance: 'circle' })],
        declarations: [ContainerComponent],
        providers: [{ provide: PLATFORM_ID, useValue: 'browser' }],
      }).createComponent(ContainerComponent);
      fixture.detectChanges();
    }),
  );

  it('should render the component using given forRoot() config', () => {
    expect(fixture.nativeElement.querySelectorAll('.skeletons-defaults .loader.circle').length).toBe(1);
  });

  it('should NOT call console.error() method', () => {
    expect(console.error).toHaveBeenCalledTimes(0);
  });

  it('should NOT call console.log() method', () => {
    expect(console.log).toHaveBeenCalledTimes(0);
  });

  it('should NOT call console.warn() method', () => {
    expect(console.warn).toHaveBeenCalledTimes(0);
  });

  it('should NOT call console.info() method', () => {
    // tslint:disable-next-line: no-console
    expect(console.info).toHaveBeenCalledTimes(0);
  });
});

Looks good, @HunteRoi . It covers the aspects of module and the existent tests are covering the defaults already

[dev054]

What happens if someone tries to use .forRoot(...) in multiple places? Is it a thing that we should care or it doesn't even matter?

[willmendesneto]

@HunteRoi I just added a comment about the tests here (#77 (comment)). Also, you have to update this line here (https://github.com/willmendesneto/ngx-skeleton-loader/blob/master/package.json#L52) (in this pull request) to be aligned with the bundle size increase and it's good to be merged and published.

[HunteRoi]

@HunteRoi I just added a comment about the tests here (#77 (comment)).

Perfect, I will add that soon!

Also, you have to update this line here (https://github.com/willmendesneto/ngx-skeleton-loader/blob/master/package.json#L52) (in this pull request) to be aligned with the bundle size increase

To what should I change this value to? Also, do you have any tests on your CI that would fail if some of the configuration are not changed?

[willmendesneto]

Also, do you have any tests on your CI that would fail if some of the configuration are not changed?

Yes, there is. You can find all the details and automated steps in this file (https://github.com/willmendesneto/ngx-skeleton-loader/blob/master/.circleci/config.yml). For some reason this pull request is not starting the pipeline, the reason why you can't see the Circle CI checks here.

At the time being, I'm merging this pull request and applying the required change since it's a small one. Once again, thanks @HunteRoi - and @dev054 for the review!

[HunteRoi]

Did you make a release recently? @willmendesneto

[willmendesneto]

@HunteRoi @dev054 Just published a new version of ngx-skeleton-loader. Please update your project to use ngx-skeleton-loader@2.10.0 to use the latest changes in your project 🚀