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

Jump to bottom

Check for treatment and guard against malformed JSON in featured extension experiment #178180

Merged
merged 1 commit into from Mar 24, 2023
Merged

Check for treatment and guard against malformed JSON in featured extension experiment #178180

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 8 additions & 4 deletions src/vs/workbench/contrib/welcomeGettingStarted/browser/featuredExtensionService.ts
View file
Open in desktop
Expand Up @@ -59,7 +59,7 @@ export class FeaturedExtensionsService extends Disposable implements IFeaturedEx

await this._init();

let treatments = this.treatment?.extensions.filter(extension => !this.ignoredExtensions.has(extension)) ?? new Array<string>();
let treatments = this.treatment?.extensions?.filter(extension => !this.ignoredExtensions.has(extension)) ?? new Array<string>();
const featuredExtensions: IFeaturedExtension[] = new Array();

if (this.treatment?.showAsList !== 'true' && treatments.length > 0) {
Expand Down Expand Up @@ -94,12 +94,16 @@ export class FeaturedExtensionsService extends Disposable implements IFeaturedEx
new Promise<string | undefined>(resolve => setTimeout(() => resolve(''), 2000))
]);

this.treatment = extensions ? JSON.parse(extensions) : { extensions: [] };
try {
this.treatment = extensions ? JSON.parse(extensions) : { extensions: [] };
} catch {
}

this.title = extensionListTitle ?? localize('gettingStarted.featuredTitle', 'Featured');

if (this.treatment) {
if (this.treatment?.extensions && Array.isArray(this.treatment.extensions)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this might regress the fix that you introduced for #177945 because this.treatment.extensions isn't guaranteed to be an array (depending on what comes back from the experimentation service)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Array.isArray takes care of the check for extensions being an array before the for loop iterates on it. Tested it with sample strings.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just for my understanding, do you know what the exp service was returning when #177945 repros? (i.e. was it just an empty response which could be safely skipped, or was it returning extension info in a different shape like an object rather than an array)? I'm mainly wondering if the Array.isArray check would cause us to not feature extensions if the exp service happens to return objects (which Object.values would still be able to extract).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was returning an empty response '{}' for a very small control group (2%). I don't believe my first fix handled it in its entirety.
This can be safely skipped and it will not cause us to not feature extensions.

const installed = await this.extensionManagementService.getInstalled();
for (const extension of Object.values(this.treatment.extensions)) {
for (const extension of this.treatment.extensions) {
if (installed.some(e => ExtensionIdentifier.equals(e.identifier.id, extension))) {
this.ignoredExtensions.add(extension);
}
Expand Down