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

fix(ontology): set the cache earlier in case of only one ontology (DSP-1374) #397

Merged
merged 15 commits into from Mar 2, 2021

Conversation

kilchenmann
Copy link
Contributor

resolves DSP-1374

@kilchenmann kilchenmann self-assigned this Feb 25, 2021
@kilchenmann kilchenmann added the bug A bug fix label Feb 25, 2021
@kilchenmann
Copy link
Contributor Author

Thank you @mdelez for approving. But @flavens figured out that with some ontologies we get some errors in the developer tools' console. I have to refactor the cache concept in the ontology.component.

@kilchenmann kilchenmann marked this pull request as draft February 25, 2021 13:40
@kilchenmann kilchenmann marked this pull request as ready for review February 26, 2021 10:30
@kilchenmann
Copy link
Contributor Author

@flavens @waychal @mdelez I finished this task. It would be nice if someone can do the review (again). Thanks

Comment on lines 251 to 253
async loadAndCache(ontologies: OntologyMetadata[]) {
await this.asyncForEach(ontologies, async (onto: OntologyMetadata) => {
await this.waitFor(200);
Copy link
Collaborator

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?

Copy link
Contributor

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?

Copy link
Contributor Author

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?

Copy link
Collaborator

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

Copy link
Contributor

@waychal waychal Feb 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One way is:

  1. set Boolean variable to false
  2. once your data is ready, set this variable to true
  3. 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try that...

Copy link
Collaborator

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.

Without waitFor:
Screen Shot 2021-02-26 at 16 44 37

With waitFor:
Screen Shot 2021-02-26 at 16 47 18

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @mdelez
I'm open for better solution than async/await. At the moment I'm lost. Even with the solution from @waychal I'm not 100% successful (yet). I'm pretty sure I'm doing something wrong, but I have no idea what and how to solve it 😞

Copy link
Contributor Author

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).

src/app/project/ontology/ontology.component.ts Outdated Show resolved Hide resolved
@@ -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();
Copy link
Collaborator

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in 68380d1

Copy link
Contributor

@flavens flavens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in general, it looks good. I have just spotted one error message when I open the Leibniz onotology (and only this one):
Screenshot 2021-03-01 at 13 36 26
The error may come from somewhere else than the cache or your code (?!).

@kilchenmann
Copy link
Contributor Author

in general, it looks good. I have just spotted one error message when I open the Leibniz onotology (and only this one):
Screenshot 2021-03-01 at 13 36 26
The error may come from somewhere else than the cache or your code (?!).

@flavens no, you're right. It was a logical issue. Resolved in 2be2a49

@kilchenmann kilchenmann requested a review from mdelez March 2, 2021 06:13
Copy link
Collaborator

@mdelez mdelez left a 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!

Copy link
Contributor

@waychal waychal left a 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.

@kilchenmann
Copy link
Contributor Author

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.

@kilchenmann kilchenmann merged commit c23ae61 into main Mar 2, 2021
@kilchenmann kilchenmann deleted the wip/dsp-1374-ontology-loader branch March 2, 2021 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants