Association decorators should not show error notifications #2165
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hudson-newey
added
bug
hudson-newey
force-pushed
the
suppress-association-errors
branch
from
2a827ec to
7a17d7e
Compare
|
Size Change: +580 B (+0.02%) Total Size: 3.59 MB
ℹ️ View Unchanged
|
atruskie
requested changes
Nov 7, 2024
| // we use a different injector for associations so that the the we can | ||
| // inject the correct options for the baw-api service without affecting | ||
| // the options for the rest of the application | ||
| const injector: Injector = parent["injector"]; |
There was a problem hiding this comment.
can we type check or guard (on name?) this somehow?
There was a problem hiding this comment.
We can acomplish compile time type-checking with some fancy TypeScript branded types
We can't directly check the name property because injector types are broadened during compile time to Injector (losing name information)
The name property is also removed during runtime
Edit: This was definately the correct call. I found so many bugs when I changed the TS type
There was a problem hiding this comment.
Related commit
src/app/services/association-injector/association-injector.service.ts
Outdated
Show resolved
Hide resolved
| @Inject(CACHE_SETTINGS) private cacheSettings: CacheSettings | ||
| @Inject(CACHE_SETTINGS) private cacheSettings: CacheSettings, | ||
| @Inject(ASSOCIATION_INJECTOR.token) protected associationInjector: any, | ||
| @Optional() @Inject(BAW_SERVICE_OPTIONS) private options: BawServiceOptions |
There was a problem hiding this comment.
ok, flatten options with default in the ctor and store the merged result
- default options: static and frozen
- instance options: Object.assign({}, defaultOptions, options)
- and ideally options that are set instance wide are not configurable in the methods
Now that we have configurable services, do we need the options configurable from individual methods? Where is disableNotificaitons used elsewhere?
There was a problem hiding this comment.
disableNotification is used in the security.service when we try to get the currently logged in user through a GET request.
Obviously in this case, we don't want to raise an error to the user if the user is not logged in, causing fetching the current user to fail.
There was a problem hiding this comment.
if that's the only other case, could we provide a service configured in the correct way and remove the method overload?
There was a problem hiding this comment.
Associated issue: #2166
atruskie
approved these changes
Nov 11, 2024
| @@ -51,7 +53,7 @@ class AnnotationSearchComponent | |||
| protected config: NgbPaginationConfig, | |||
| protected modals: NgbModal, | |||
| protected annotationService: AnnotationService, | |||
| private injector: Injector | |||
| @Inject(ASSOCIATION_INJECTOR) private injector: AssociationInjector | |||
There was a problem hiding this comment.
ok this is a bit of a smell. the only place the association injector should be needed is from services that create models.
we should also be injecting this injector in creation of whatever is using it - the search parameters
There was a problem hiding this comment.
Related issue: #2167
| @@ -75,7 +77,7 @@ class VerificationComponent | |||
| private route: ActivatedRoute, | |||
| private router: Router, | |||
| private location: Location, | |||
| private injector: Injector | |||
| @Inject(ASSOCIATION_INJECTOR) private injector: AssociationInjector | |||
| @@ -146,7 +148,9 @@ export class FolderRowComponent { | |||
| return this.harvestItem.report; | |||
| } | |||
|
|
|||
| public constructor(private injector: Injector) {} | |||
| public constructor( | |||
| @Inject(ASSOCIATION_INJECTOR) protected injector: AssociationInjector, | |||
| @@ -95,7 +97,7 @@ class PermissionsComponent | |||
| private permissionsApi: PermissionsService, | |||
| private accountsApi: AccountsService, | |||
| private route: ActivatedRoute, | |||
| private injector: Injector | |||
| @Inject(ASSOCIATION_INJECTOR) private injector: AssociationInjector, | |||
| IParameterModel<EventSummaryReport> | ||
| { | ||
| public constructor( | ||
| queryStringParameters: Params = {}, | ||
| public injector?: Injector | ||
| public injector?: AssociationInjector |
|
|
||
| // this brand symbol should never be used at runtime | ||
| // it is only used to have a type that cannot be instantiated by TypeScript | ||
| // outside of the Brand function |
There was a problem hiding this comment.
and the purpose of this is?
To create something like a type alias that is not-equivalent as far as TS is concerned?
There was a problem hiding this comment.
Discussed offline that this needed an inline code comment
| import { Brand } from "@helpers/advancedTypes"; | ||
| import { AbstractModel } from "./AbstractModel"; | ||
|
|
||
| export type AssociationInjector = Brand<Injector, "AssociationInjector">; |
There was a problem hiding this comment.
this is very fancy, but I wonder if you couldn't have just subclassed injector?
I guess with brand there's no runtime difference
There was a problem hiding this comment.
There are a couple of benefits to using a branded type
- There is no runtime difference (reducing the risk of bugs)
- Using a subclassed injector risks polymorphic type matching
e.g.
export class AssociationInjector extends Injector {}
console.log(AssociationInjector === Injector);
// ^ will not throw a compile time error unless we add a branding property at compile time or run time
// resulting in the same solution, just with an additional class that we have to maintain
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Association decorators should not show error notifications
This pull request creates a new association injector that can be used to inject different options into the
baw-api.service.tsChanges
association-injector.service. This injector used in the abstract model constructorBawAptOptionsto thebaw-api.serviceServiceToken.kindServiceProvidersandAbstractDataModelPageInfotests to annotation search pageIssues
Fixes: #1967
Final Checklist
npm run lint)npm run test:all)