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

Feature/#703 typed injectiontokens #704

Merged

Conversation

hwanders
Copy link
Contributor

New Pull Request

Closes #703

Prerequisites

Please make sure you can check the following boxes:

  • I have read the CONTRIBUTING document
  • My code follows the code style of this project (I hope so)
  • All new and existing tests passed (see below)

Type(s) of Changes

Contribution Type

What types of changes does your code introduce? Put an x in all the boxes that apply:

  • Bug fix (non-breaking change which fixes an issue, please reference the issue id)
  • New feature (non-breaking change which adds functionality, make sure to open an associated issue first)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • I have updated the documentation accordingly
  • I have added tests to cover my changes (see below)

Description

This adds new InjectionToken constants to piral-ng and additionally registers the known services using these tokens.
This improves compatibility with Angular's inject function.

Remarks

I've written this PR without actually being a consumer of this project. So my understanding of it is limited.

Existing tests: on my system some tests have failed before the change, still failing after the change:

  • src/tooling/piral-cli/src/common/npm.test.ts > npm Module > makeFilePath returns valid file path
  • src/tooling/piral-cli/src/common/npm.test.ts > npm Module > gets path to file package
  • src/tooling/piral-cli/src/common/utils.test.ts > only unique > should filter out child directories from back

New tests: Setting up an automated test for piral-ng is currently beyond my scope / understanding of this project.
A reasonable test (be it manual or automated) would ensure:

  • Using the new tokens in inject survives the type check (in contrast to the existing string based tokens)
  • Using the new tokens in inject or @Inject really leads to injection of the services

Injection token types: I'm also a bit unsure about the typing of the PROPS token. Currently it's InjectionToken<any>.
Without much experience in this project, it seems to me that almost anything can be passed here. Perhaps any can be improved to Record<string, any> or something similar but a simple any should make sure the consumer can just write down their own props type on the @Inject-decorated parameter / inject-assigned field.

Injection token names:: I've reproduced the original service names (uppercase) to name the exported injection token constants. The names passed to the InjectionToken constructor are just for debugging purposes.

Feel free to edit anything in this PR.

@CLAassistant
Copy link

CLAassistant commented Jul 20, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

File size impact

Merging feature/#703-typed-injectiontokens into develop impact files as follow:

empty-piral (no impact)
Files new size
Unmodified (2) 30.2 kB (0 B / +0%) 👻
Total (2) 30.2 kB (0 B / +0%) 👻
minimal-piral (no impact)
Files new size
Unmodified (2) 277 kB (0 B / +0%) 👻
Total (2) 277 kB (0 B / +0%) 👻
sample-cross-fx (+0.01%)
Files new size
src/samples/sample-cross-fx/dist/release/index.bfa38c.js 2.22 MB 👶
src/samples/sample-cross-fx/dist/release/main.4c0723.css 1.67 kB 👶
src/samples/sample-cross-fx/dist/release/main.40c965.css deleted (-1.67 kB)
src/samples/sample-cross-fx/dist/release/index.66af9a.js deleted (-2.22 MB)
src/samples/sample-cross-fx/dist/release/index.html 454 B (0 B / +0%) 👻
Total (5) 2.22 MB (+142 B / +0.01%) ↗️
sample-piral (no impact)
Files new size
Unmodified (3) 696 kB (0 B / +0%) 👻
Total (3) 696 kB (0 B / +0%) 👻
sample-piral-core (no impact)
Files new size
Unmodified (3) 355 kB (0 B / +0%) 👻
Total (3) 355 kB (0 B / +0%) 👻
Generated by @jsenv/file-size-impact during check-bundle-size#10019928510 on e372f63

@FlorianRappl
Copy link
Contributor

Thanks a lot - all tests are working as they should.

Much appreciated!

@FlorianRappl FlorianRappl added the angular Concerns the Angular integration piral-ng. label Jul 20, 2024
@FlorianRappl FlorianRappl added this to the 1.6.1 milestone Jul 20, 2024
@FlorianRappl FlorianRappl merged commit b74cad3 into smapiot:develop Jul 20, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
angular Concerns the Angular integration piral-ng.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

InjectionToken<...> for public providers in piral-ng
3 participants