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.

Sign up for GitHub

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

Moving CDN PreFetch to kick off prior to initialize call and added timeout logic #2666

Merged
merged 28 commits into from
Jan 24, 2025
Merged

Moving CDN PreFetch to kick off prior to initialize call and added timeout logic #2666

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
7840b15
Merge branch 'main' of https://github.com/OfficeDev/microsoft-teams-l…
jadahiya-MSFT Nov 7, 2024
0e63ccf
Merged in main
jadahiya-MSFT Nov 7, 2024
521b734
Merge branch 'main' of https://github.com/OfficeDev/microsoft-teams-l…
jadahiya-MSFT Nov 20, 2024
793c039
Merge branch 'main' of https://github.com/OfficeDev/microsoft-teams-l…
jadahiya-MSFT Dec 4, 2024
a30024f
Merge branch 'main' of https://github.com/OfficeDev/microsoft-teams-l…
jadahiya-MSFT Dec 13, 2024
a17d3d1
Merge branch 'main' of https://github.com/OfficeDev/microsoft-teams-l…
jadahiya-MSFT Dec 30, 2024
edfa7f3
Merge branch 'main' of https://github.com/OfficeDev/microsoft-teams-l…
jadahiya-MSFT Jan 2, 2025
c3addae
Moving prefetch of CDN resources to earlier in the pipeline and addin…
jadahiya-MSFT Jan 2, 2025
5f2faab
Added in changefile
jadahiya-MSFT Jan 2, 2025
a2a0ae1
Added in prefetch file to minimize side effects
jadahiya-MSFT Jan 2, 2025
0d919f7
Updated changelog
jadahiya-MSFT Jan 2, 2025
0f6483a
Moved prefetch to public
jadahiya-MSFT Jan 2, 2025
2beeffc
Added setupTest file to add global fetch polyfill to Jest tests and m…
jadahiya-MSFT Jan 3, 2025
b21fc8d
Added comment describing why fetch is polyfilled
jadahiya-MSFT Jan 3, 2025
74a6e89
Moved prefetch logic into validOrigins logic since validOrigins file …
jadahiya-MSFT Jan 6, 2025
3e85df8
Created disableCache mechanism so that validOrigins unit tests will g…
jadahiya-MSFT Jan 7, 2025
2e94704
Created disableCache mechanism so that validOrigins unit tests will g…
jadahiya-MSFT Jan 7, 2025
7fe8c40
Removed unneeded error variable in logger call
jadahiya-MSFT Jan 7, 2025
d8ccea5
Added in unit tests
jadahiya-MSFT Jan 8, 2025
9d66560
Merge branch 'main' into jadahiya/ddl_fix
jadahiya-MSFT Jan 8, 2025
381ce3e
Merge branch 'jadahiya/ddl_fix' of https://github.com/OfficeDev/micro…
jadahiya-MSFT Jan 8, 2025
d9704f3
Merge branch 'main' into jadahiya/ddl_fix
jadahiya-MSFT Jan 9, 2025
da38ebf
Merge branch 'main' into jadahiya/ddl_fix
jadahiya-MSFT Jan 13, 2025
cb29943
Merge branch 'main' into jadahiya/ddl_fix
jadahiya-MSFT Jan 23, 2025
3034cd5
Merge branch 'jadahiya/ddl_fix' of https://github.com/OfficeDev/micro…
jadahiya-MSFT Jan 23, 2025
34bde9d
Fixed typo and renamed disableCache to shouldDisableCache
jadahiya-MSFT Jan 24, 2025
c3097b4
Moving timeout to constants file and exporting it
jadahiya-MSFT Jan 24, 2025
3bc52b5
Merge branch 'main' into jadahiya/ddl_fix
jadahiya-MSFT Jan 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions packages/teams-js/src/internal/validOrigins.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { inServerSideRenderingEnvironment, isValidHttpsURL } from './utils';

let validOriginsCache: string[] = [];
const validateOriginLogger = getLogger('validateOrigin');
const ORIGIN_LIST_TIMEOUT = 1500;
jadahiya-MSFT marked this conversation as resolved.
Show resolved Hide resolved

export async function prefetchOriginsFromCDN(): Promise<void> {
await getValidOriginsListFromCDN();
Expand All @@ -19,7 +20,7 @@ async function getValidOriginsListFromCDN(): Promise<string[]> {
return validOriginsCache;
}
if (!inServerSideRenderingEnvironment()) {
return fetch(validOriginsCdnEndpoint)
return fetch(validOriginsCdnEndpoint, { signal: AbortSignal.timeout(ORIGIN_LIST_TIMEOUT) })
jadahiya-MSFT marked this conversation as resolved.
Show resolved Hide resolved
.then((response) => {
if (!response.ok) {
throw new Error('Invalid Response from Fetch Call');
Expand All @@ -34,7 +35,11 @@ async function getValidOriginsListFromCDN(): Promise<string[]> {
});
})
.catch((e) => {
validateOriginLogger('validOrigins fetch call to CDN failed with error: %s. Defaulting to fallback list', e);
if (e.name === 'TimeoutError') {
validateOriginLogger('validOrigins fetch call to CDN failed due to Timeout. Defaulting to fallback list', e);
jadahiya-MSFT marked this conversation as resolved.
Show resolved Hide resolved
jadahiya-MSFT marked this conversation as resolved.
Show resolved Hide resolved
} else {
jadahiya-MSFT marked this conversation as resolved.
Show resolved Hide resolved
validateOriginLogger('validOrigins fetch call to CDN failed with error: %s. Defaulting to fallback list', e);
}
validOriginsCache = validOriginsFallback;
return validOriginsCache;
});
Expand Down
3 changes: 2 additions & 1 deletion packages/teams-js/src/public/app/app.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ import {
import { version } from '../version';
import * as lifecycle from './lifecycle';

prefetchOriginsFromCDN();
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that this is called in the global namespace this file has a side effect that it didn't used to have (loading the file in memory now automatically makes a call to the CDN). Can you verify with Noah that we don't need to mark this file in any particular way to prevent it from being treeshaken out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, I'll follow up with Noah on this and if there's any issue I'll move this call into a separate funciton.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Followed up with Noah, this will in fact introduce an unintended side effect that would make app.ts not treeshake-able. I'll be pushing out a change that will mitigate this or lessen the impact of the side effect.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there something we can do so issues like this are apparent to developers who introduce them without requiring @TrevorJoelHarris or @noahdarveau-MSFT to be manually looking for them in code reviews? Certainly I wouldn't have realized this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm, that's a good question. I'll think about it (and maybe ask @noahdarveau-MSFT). In general, any function calls in the global namespace are "side effects" if they cause some state somewhere to change because those functions will just get called assuming the entire module doesn't get tree-shaken out and removed. Off the top of my head I'm not sure there's a way to "detect" that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I am pretty sure that we could make a linter rule to detect global function calls that would show a warning saying something like "This file should probably be added to the sideEffects array in package.json. Once you've done that (or verified that you don't need to) please suppress this warning."


/**
* v2 APIs telemetry file: All of APIs in this capability file should send out API version v2 ONLY
*/
Expand Down Expand Up @@ -615,7 +617,6 @@ logWhereTeamsJsIsBeingUsed();
* @returns Promise that will be fulfilled when initialization has completed, or rejected if the initialization fails or times out
*/
export function initialize(validMessageOrigins?: string[]): Promise<void> {
prefetchOriginsFromCDN();
return appHelpers.appInitializeHelper(
getApiVersionTag(appTelemetryVersionNumber, ApiName.App_Initialize),
validMessageOrigins,
Expand Down
Loading