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

feat(launcher): check dependencies before launch on Windows #3240

Merged
merged 7 commits into from
Jul 31, 2020

Conversation

yury-s
Copy link
Member

@yury-s yury-s commented Jul 30, 2020

Adding PrintDeps.exe to the npm changed size of playwright.tgz 419K => 537K

#2745

`https://support.microsoft.com/en-us/help/2977003/the-latest-supported-visual-c-downloads`,
``,
].join('\n'));
break;
Copy link
Collaborator

@aslushnikov aslushnikov Jul 30, 2020

Choose a reason for hiding this comment

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

why breaking and doing another loop down below? Instead, you can do

const knownIssues = new Set(); // to avoid dupes!
for (const dep of missingDeps) {
  if (dep.startsWith('api-ms-win-crt-')) {
    // ....
    knownIssues.add('one known issue');
  } else if (dep === "mf.dll" || dep === "mfplat.dll" ||  dep === "msmpeg2vdec.dll") {
    // ...
    knownIssues.add('another known issue');
  }
}

return;

let isCrtMissing = false;
Copy link
Collaborator

@aslushnikov aslushnikov Jul 30, 2020

Choose a reason for hiding this comment

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

nit:

const isCrtMissing = [...missingDeps].some(dep => dep.startsWith('api-ms-win-crt'));
const isMediaFoundationMissing = [...missingDeps].some(dep => ['mf.dll', 'mfplat.dll', 'msmpeg2vdec.dll'].includes(dep));

(and then it can even be inlined)

@yury-s yury-s merged commit cbfdca7 into microsoft:master Jul 31, 2020
@yury-s yury-s deleted the check-deps-win branch July 31, 2020 00:15
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