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
Collaborator

@kilchenmann kilchenmann commented Feb 25, 2021

resolves DSP-1374

@kilchenmann kilchenmann self-assigned this Feb 25, 2021
@kilchenmann kilchenmann requested review from flavens, mdelez and waychal Feb 25, 2021
mdelez
mdelez approved these changes Feb 25, 2021
@kilchenmann
Copy link
Collaborator Author

@kilchenmann kilchenmann commented Feb 25, 2021

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.

Loading

@kilchenmann kilchenmann marked this pull request as draft Feb 25, 2021
@kilchenmann kilchenmann marked this pull request as ready for review Feb 26, 2021
@kilchenmann kilchenmann requested a review from mdelez Feb 26, 2021
@kilchenmann
Copy link
Collaborator Author

@kilchenmann kilchenmann commented Feb 26, 2021

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

Loading

async loadAndCache(ontologies: OntologyMetadata[]) {
await this.asyncForEach(ontologies, async (onto: OntologyMetadata) => {
await this.waitFor(200);
Copy link
Contributor

@mdelez mdelez Feb 26, 2021

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?

Loading

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.

Whats happens if wait is over but data is still not ready?

Loading

Copy link
Collaborator Author

@kilchenmann kilchenmann Feb 26, 2021

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?

Loading

Copy link
Contributor

@mdelez mdelez Feb 26, 2021

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

Loading

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

Loading

Copy link
Collaborator Author

@kilchenmann kilchenmann Feb 26, 2021

Choose a reason for hiding this comment

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

I'll try that...

Loading

Copy link
Contributor

@mdelez mdelez Feb 26, 2021

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.

Loading

Copy link
Collaborator Author

@kilchenmann kilchenmann Feb 26, 2021

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 😞

Loading

Copy link
Collaborator Author

@kilchenmann kilchenmann Feb 26, 2021

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

Loading

src/app/project/ontology/ontology.component.ts Outdated Show resolved Hide resolved
Loading
@kilchenmann kilchenmann requested a review from mdelez Feb 26, 2021
@@ -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
Contributor

@mdelez mdelez Mar 1, 2021

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

Loading

Copy link
Collaborator Author

@kilchenmann kilchenmann Mar 2, 2021

Choose a reason for hiding this comment

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

done in 68380d1

Loading

flavens
flavens approved these changes Mar 1, 2021
Copy link
Collaborator

@flavens flavens left a comment

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 (?!).

Loading

@kilchenmann
Copy link
Collaborator Author

@kilchenmann kilchenmann commented Mar 2, 2021

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

Loading

@kilchenmann kilchenmann requested a review from mdelez Mar 2, 2021
mdelez
mdelez approved these changes Mar 2, 2021
Copy link
Contributor

@mdelez mdelez left a comment

Looks good to me!

Loading

waychal
waychal approved these changes Mar 2, 2021
Copy link
Contributor

@waychal waychal left a comment

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.

Loading

@kilchenmann
Copy link
Collaborator Author

@kilchenmann kilchenmann commented Mar 2, 2021

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.

Loading

@kilchenmann kilchenmann merged commit c23ae61 into main Mar 2, 2021
6 checks passed
Loading
@kilchenmann kilchenmann deleted the wip/dsp-1374-ontology-loader branch Mar 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants