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
fix(ontology): set the cache earlier in case of only one ontology (DSP-1374) #397
Conversation
async loadAndCache(ontologies: OntologyMetadata[]) { | ||
await this.asyncForEach(ontologies, async (onto: OntologyMetadata) => { | ||
await this.waitFor(200); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clarify why this method is needed? I'm not a huge fan of waitFor
and we have stated in our design doc that we shouldn't use setTimeout in production code. How will this behave with a rather slow internet connection?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whats happens if wait is over but data is still not ready?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I know, that we have this rule in our conventions. But I have no idea how to solve this async requests in another way. I found this solution in a tutorial. Maybe someone of you has a better idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll pull the changes from main and check it out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One way is:
- set Boolean variable to false
- once your data is ready, set this variable to true
- add watch on the boolean variable defined above and so when it becomes true, call the function you want run after the Ajax data is ready
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try that...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idk if you've already found this @kilchenmann but the issue isn't that the data isn't arriving in time from the API, it's that this.ontologies
isn't modified in time for the length check within loadAndCache
so its length is always 0.
I think it's due to your callback. Admittedly I'm quite horrible at dealing with async/await things but I'm quite certain it's an issue with your callback getting triggered earlier than it should.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I came up with another solution without async/await. See my last commit (d174ba2).
@@ -375,7 +362,7 @@ export class OntologyComponent implements OnInit { | |||
openResourceClassForm(mode: 'createResourceClass' | 'editResourceClass', resClassInfo: DefaultClass): void { | |||
|
|||
// set cache for ontology and lists | |||
this.setCache(); | |||
// this.setCache(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be removed? Same with the other similar commented out line below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in 68380d1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In file src/app/project/ontology/ontology.component.html
line 45: according to DSP-APP code conventions class attribute should be at the end.
Everything else looks fine to me.
Thanks for the info. I think this HTML code convention is not yet fully integrated in DSP-APP. We should plan a separat HTML refactoring task for this issue. |
resolves DSP-1374