fix(client): Only allow one profile request per account per tab session.

[shane-tomlinson]

This avoids multiple XHR requests being made by multiple views when displaying
profile information at the same time.

issue #3053

@zaach - r?

I'm not fully convinced by this PR, but it does remove multiple profile requests being made when settings starts up.

[zaach]

We'd need some way to invalidate the cache with this approach. You could clear the promise once it resolves/rejects, which would prevent simultaneous requests but still allow it to fetch new data in the future.

[shane-tomlinson]

We'd need some way to invalidate the cache with this approach. You could clear the promise once it resolves/rejects, which would prevent simultaneous requests but still allow it to fetch new data in the future.

This was the exact original approach I took, and I took it out because 2 profile requests were still made when loading /settings. The first finished before the second started.

• the first -

  fxa-content-server/app/scripts/views/settings.js

  Line 128 in f3b6aa1

  return account.fetchProfile()

• the second -

  fxa-content-server/app/scripts/views/settings/display_name.js

  Line 36 in f3b6aa1

  return account.fetchProfile()

Removing the first seems to have no ill effects even with an avatar set. I don't know the code well enough to fully understand why. My guess is when the 2nd completes, the profile change notification is triggered, and settings.js->onProfileUpdate is called, which renders the avatar.

[zaach]

My guess is when the 2nd completes, the profile change notification is triggered, and settings.js->onProfileUpdate is called, which renders the avatar.

So the main issue might be around that– if another fetch happens after a profile update it seems like it'll still return the cached data. Perhaps the cache should be cleared after an update notification is received. There's also an inconsistency with fetchCurrentProfileImage, which is not cached and could conceivably return a different image url. Probably an edge case...

An alternative would be to fetch the profile data in settings then make it available to the sub-panels, (which is possible through the view.super reference). A method for getting the profile data could be added to the settings subview mixin.

Long-term I think the caching approach is probably better, with the edge cases worked out.

[shane-tomlinson]

Perhaps the cache should be cleared after an update notification is received.

Do you mean if we receive notification from another tab? That's entirely reasonable.

There's also an inconsistency with fetchCurrentProfileImage, which is not cached and could conceivably return a different image url.

We could cache it, but that still doesn't get around the inconsistency. We could invalidate the cache then, or create a new profile promise with the updated data. Simply invalidating the cache would be easier.

[shane-tomlinson]

@zaach - could we also have fetchCurrentProfileImage call fetchProfile, get the image, do a fetch on the image, and return that? It seems like at a minimum the internal state would always stay consistent.

As an alternative, we could cause fetchCurrentProfileImage to invalidate the profile promise iff the profile image id and url do not match what is already stored.

[shane-tomlinson]

@zaach - yet another alternative is to have the model listen for its own changed messages, or override set. When anything is updated, clear the cache. This seems gross, but it'd work.

[zaach]

We don't really have a story for handling out of bound profile changes, either. Currently it's possible to see one profile image on the settings page and a different, newer profile image on the avatar change page if the image was updated in a separate browser between the loading of each.

With a cached version they would always be consistent, if outdated. Trying to delete an outdated cached version would be problematic– you may think you've cleared your profile image but there could be a newer one still set as your default.

[zaach]

yet another alternative is to have the model listen for its own changed messages, or override set. When anything is updated, clear the cache. This seems gross, but it'd work.

Yeah, I think we have to do something like that. Otherwise, whatever was resolved in that cached promise would overwrite the current state of the model if fetchProfile is called again.

could we also have fetchCurrentProfileImage call fetchProfile

The difference between fetchCurrentProfileImage and fetchProfile is that fetchProfile doesn't return the avatar's ID, which is needed to delete it. I've filed mozilla/fxa-profile-server#149 about that.

[vladikoff]

needs @zaach help

[shane-tomlinson]

This is no longer WIP, @zaach, I think this approach is workable now! Thanks for the tips/review earlier, mind a re-review?

[zaach]

I like it!