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

Remove protected access modifier from request method in CollectionService #1139

Closed
2 of 8 tasks
danipavic opened this issue Jan 9, 2023 · 0 comments · Fixed by #1140
Closed
2 of 8 tasks

Remove protected access modifier from request method in CollectionService #1139

danipavic opened this issue Jan 9, 2023 · 0 comments · Fixed by #1140

Comments

@danipavic
Copy link
Contributor

Relevant version

  • 0.x
  • 1.x
  • 2.x

Relevant libraries

  • utils
  • core
  • network
  • jsonapi
  • jsonapi-angular

Breaking change

No

Description

Currently request method in CollectionService has protected access modifier. This isn't causing any issues on its own but makes writing hacky unit tests for model services that use request.

Example of a real method in a service that extends CollectionService:

@Injectable({
	providedIn: 'root',
})
export class ExampleService extends CollectionService<ExampleModel, AppCollection> {
	constructor(@Inject(APP_COLLECTION) protected override readonly collection: AppCollection) {
		super(collection);
	}

	public exampleMethod(): Observable<Response<ExampleModel>> {
		return this.request(`quizzes/${id}/finish`, 'PATCH');
	}
}

Since the request is protected it is not possible to spyOn without casting and the tests look like this:

	it('should finish', (done: DoneFn) => {
		const requestSpy = spyOn(service, 'request' as any).and.returnValue(of({}));
	
		service.exampleMethod().subscribe(() => {
			expect(requestSpy).toHaveBeenCalledOnceWith(`quizzes/${id}/finish`, 'PATCH');
			done();
		});
	});

I would like to avoid as any, I can't think of any reason why this method would be protected as all other methods are public, and also the request method in IJsonapiCollection interface is public as well. TLDR: We should remove the access modifier on request method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants