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 appId parameter to getContext #2337

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

Conversation

vikramtha
Copy link
Contributor

@vikramtha vikramtha commented May 22, 2024

Description

This is adding the appId parameter to getContext. This is what hubs use to differentiate the app internally as well as what ends up being used in deeplinking

Here is the PM Spec for this: https://microsoft.sharepoint-df.com/:w:/t/MetaOS/ESh4ef-uxiBPohWO0I6DszoBzu6VGd_06r-Xg3TIy9tT8w?e=RKfWzE

If this Pull Request should close/resolve any issues when merged, use the special syntax for that here.

Main changes in the PR:

  1. Added appId to getContext() and the AppInfo interface
  2. Made sure unit tests passed for the appId through the context bridge

Validation

Validation performed:

  1. Validated on Orange Web using both the Teams Test App and the Test App in the teamsJS sdk

Unit Tests added:

Unit tests are required for all changes. If no unit tests were added as part of this change, please explain why they aren't necessary.

Yes added some dummy values in the test files. No E2E tests added

End-to-end tests added:

No

Additional Requirements

Change file added:

Yes

@vikramtha vikramtha marked this pull request as ready for review May 22, 2024 23:58
@vikramtha vikramtha requested a review from a team as a code owner May 22, 2024 23:58
@vikramtha vikramtha marked this pull request as draft May 23, 2024 06:17
@vikramtha vikramtha marked this pull request as ready for review June 10, 2024 22:30
@vikramtha vikramtha requested a review from Ella-ly June 10, 2024 22:31
@TrevorJoelHarris
Copy link
Contributor

@vikramtha this is failing to build, so it won't get any PR reviews until the build is passing

Copy link
Contributor Author

@vikramtha vikramtha left a comment

Choose a reason for hiding this comment

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

Updating

@vikramtha vikramtha changed the title Added intAppId parameter to getContext Added appId parameter to getContext Jul 2, 2024
*
* App id that is used by Hosts to distinguish between different apps sideloaded or in store
*/
appId: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a new feature, can I know why we need to add this parameter to a deprecated interface and as a deprecated element?

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 believe the Context deprecated interface is still used in the legacyContext transformation so I kept it

Copy link
Contributor

Choose a reason for hiding this comment

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

This one should probably also be optional

Ella-ly
Ella-ly previously approved these changes Jul 25, 2024
@TrevorJoelHarris TrevorJoelHarris enabled auto-merge (squash) August 13, 2024 22:15
jadahiya-MSFT
jadahiya-MSFT previously approved these changes Aug 13, 2024
@TrevorJoelHarris TrevorJoelHarris dismissed stale reviews from jadahiya-MSFT and themself via 6a26ec4 August 15, 2024 17:05
Copy link
Contributor

@maglims maglims left a comment

Choose a reason for hiding this comment

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

I haven't looked into much deeper but first question I have is: doesn't it need changes in hub sdks to populate back appId since it looks it is defined as required?

Are all hub sdk test passing?

/**
* App id that is used by Hosts to distinguish between different apps sideloaded or in store
*/
appId: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

@maglims brought up a good question that I forgot: since not all hosts will be setting this value, shouldn't it be optional (like appLaunchId)?

Copy link
Contributor Author

@vikramtha vikramtha Aug 16, 2024

Choose a reason for hiding this comment

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

This will always be populated as long as there is an app def sent back; I rewrote the code in host SDK so that hosts no longer need to pass the value and instead it is taken from app definition

Copy link
Contributor

Choose a reason for hiding this comment

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

What about hosts that aren't yet using the updated host-sdk? I believe in that case this would not be set, right? That's generally why we mark things like this optional, to handle backwards compatibility

Copy link
Contributor

@TrevorJoelHarris TrevorJoelHarris left a comment

Choose a reason for hiding this comment

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

There are some questions about appId being optional, and I think it should be. If you have questions just send me a message and we'll talk about it.

/**
* App id that is used by Hosts to distinguish between different apps sideloaded or in store
*/
appId: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

string

Can you use the AppId type for this? Strong types are better than strings. It should be easy to create this object as an AppId in transformLegacyContextToAppContext after the data comes in over the wire as a string.

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.

6 participants