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

Add additional optional chaining to prevent import errors when chrome api not available #8432

Merged
merged 1 commit into from
May 9, 2024

Conversation

mnholtz
Copy link
Collaborator

@mnholtz mnholtz commented May 9, 2024

What does this PR do?

Discussion

  • For some reason, the changes made in the previous PR were passing cypress tests in headless mode (and local builds looked fine) but Misha's cypress tests are failing in UI mode, and staging is down
  • We should seriously consider adding smoke tests to dev and staging

Checklist

  • Add jest or playwright tests and/or storybook stories
  • Designate a primary reviewer @johnnymetz

Copy link

github-actions bot commented May 9, 2024

No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack.

Do not edit this comment manually.

Copy link

codecov bot commented May 9, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 73.41%. Comparing base (6d95047) to head (120583e).
Report is 36 commits behind head on main.

Files Patch % Lines
src/mv3/api.ts 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8432      +/-   ##
==========================================
- Coverage   73.47%   73.41%   -0.06%     
==========================================
  Files        1334     1352      +18     
  Lines       41259    41520     +261     
  Branches     7686     7773      +87     
==========================================
+ Hits        30316    30483     +167     
- Misses      10943    11037      +94     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mnholtz mnholtz enabled auto-merge (squash) May 9, 2024 20:09
Comment on lines +24 to +30
if (!chrome.runtime?.getManifest) {
return false;
}

// Use optional chaining in case the chrome runtime is not available:
// https://github.com/pixiebrix/pixiebrix-extension/issues/8273
return chrome.runtime.getManifest().manifest_version === 3;
Copy link
Contributor

@BLoe BLoe May 9, 2024

Choose a reason for hiding this comment

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

I'm pretty sure you can also just keep chaining the null-safe access as before, if you want:

Suggested change
if (!chrome.runtime?.getManifest) {
return false;
}
// Use optional chaining in case the chrome runtime is not available:
// https://github.com/pixiebrix/pixiebrix-extension/issues/8273
return chrome.runtime.getManifest().manifest_version === 3;
// Use optional chaining in case the chrome runtime is not available:
// https://github.com/pixiebrix/pixiebrix-extension/issues/8273
return chrome.runtime?.getManifest?.()?.manifest_version === 3;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks ben, I actually didn't remember that you could put the double parentheses after the null operator.

Copy link

github-actions bot commented May 9, 2024

Playwright test results - MV2

passed  41 passed
flaky  1 flaky
skipped  8 skipped

Details

report  Open report ↗︎
stats  50 tests across 18 suites
duration  9 minutes, 48 seconds
commit  120583e

Flaky tests

edge › tests/extensionConsoleActivation.spec.ts › can activate a mod with a database

Skipped tests

chrome › tests/extensionConsoleActivation.spec.ts › can activate a mod with built-in integration
edge › tests/extensionConsoleActivation.spec.ts › can activate a mod with built-in integration
chrome › tests/runtime/sidebarController.spec.ts › sidebar controller › shows focus dialog in top-level frame
edge › tests/runtime/sidebarController.spec.ts › sidebar controller › shows focus dialog in top-level frame
chrome › tests/runtime/sidebarNavigation.spec.ts › sidebar mod panels are persistent during navigation
chrome › tests/runtime/sidebarNavigation.spec.ts › sidebar form and temporary panels are unavailable after navigation
edge › tests/runtime/sidebarNavigation.spec.ts › sidebar mod panels are persistent during navigation
edge › tests/runtime/sidebarNavigation.spec.ts › sidebar form and temporary panels are unavailable after navigation

Copy link

github-actions bot commented May 9, 2024

Playwright test results - MV3

passed  48 passed
flaky  2 flaky

Details

report  Open report ↗︎
stats  50 tests across 18 suites
duration  14 minutes, 29 seconds
commit  120583e

Flaky tests

chrome › tests/regressions/doNotCloseSidebarOnPageEditorSave.spec.ts › #8104: Do not automatically close the sidebar when saving in the Page Editor
edge › tests/regressions/doNotCloseSidebarOnPageEditorSave.spec.ts › #8104: Do not automatically close the sidebar when saving in the Page Editor

@mnholtz mnholtz merged commit 003dc96 into main May 9, 2024
28 checks passed
@mnholtz mnholtz deleted the bugfix/chrome_api_incompatible_with_app branch May 9, 2024 20:20
@grahamlangford grahamlangford added this to the 1.8.14 milestone May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants