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

Added resolver for perspective from url method #44

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

fayeed
Copy link
Member

@fayeed fayeed commented Jan 12, 2022

Depends on #29

@lucksus
Copy link
Member

lucksus commented Jan 16, 2022

Basically ok, though I've solved this in Perspect3ve inside the UI only by iterating over all perspectives and searching for the one with the given sharedURL, which I would suggest to do here on this level as well as it would be less error prone, since no need to update any map cache...

@lucksus
Copy link
Member

lucksus commented Jan 16, 2022

... which BTW is what is missing in this PR. The map/cache update happens in perspectiveSnapshot() and not in add() which can make the map get stale..

@fayeed
Copy link
Member Author

fayeed commented Jan 17, 2022

Basically ok, though I've solved this in Perspect3ve inside the UI only by iterating over all perspectives and searching for the one with the given sharedURL, which I would suggest to do here on this level as well as it would be less error prone, since no need to update any map cache...

Right now we are doing exactly this getting all the perspectives using the all method and looping over it. The reason I thought this method might be needed is as the users join more neighbourhoods the number of perspectives will increase and looping every time would not be great.

@lucksus
Copy link
Member

lucksus commented Jan 19, 2022

Well, I don't think looping over Perspectives will really become a problem.. Even less so if the looping would happen here in the ad4m-executor (because it would save GQL interface round-trips).

And I also don't oppose adding/merging this, but then I would like to see proper tests :)

@lucksus lucksus changed the base branch from main to dev January 5, 2023 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants