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

#9105: make getState and setState async #9122

Merged
merged 5 commits into from
Sep 9, 2024
Merged

Conversation

twschiller
Copy link
Contributor

@twschiller twschiller commented Sep 9, 2024

What does this PR do?

Discussion

  • The main consideration is that page state could be updated between get + set calls. So WithAsyncModVariable's logic needed to be rewritten to ensure a valid async state shape

For more information on our expectations for the PR process, see the
code review principles doc

@twschiller twschiller added the do not merge Merging of this PR is blocked by another one label Sep 9, 2024
@twschiller twschiller self-assigned this Sep 9, 2024
@twschiller twschiller added the enhancement New feature or request label Sep 9, 2024
@twschiller twschiller changed the title #9105: [WIP] make getState and setState async #9105: make getState and setState async Sep 9, 2024
@twschiller twschiller added this to the 2.1.2 milestone Sep 9, 2024
@twschiller twschiller marked this pull request as ready for review September 9, 2024 00:38
Base automatically changed from feature/9105-platform-state to main September 9, 2024 11:49
Copy link
Contributor Author

@twschiller twschiller Sep 9, 2024

Choose a reason for hiding this comment

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

This brick had to be updated to handle interleaving calls to get/set. The request id is now set immediately, which better matches the RTK Query behavior for request id

@twschiller twschiller removed the do not merge Merging of this PR is blocked by another one label Sep 9, 2024
Copy link

github-actions bot commented Sep 9, 2024

Playwright test results

passed  119 passed
flaky  7 flaky
skipped  6 skipped

Details

report  Open report ↗︎
stats  132 tests across 44 suites
duration  10 minutes, 59 seconds
commit  f82e25a
info  For more information on how to debug and view this report, see our readme

Flaky tests

chrome › tests/pageEditor/addStarterBrick.spec.ts › Add starter brick to mod
msedge › tests/pageEditor/addStarterBrick.spec.ts › Add starter brick to mod
msedge › tests/runtime/customEvents.spec.ts › custom event brick functionality
chrome › tests/runtime/insertAtCursor.spec.ts › Insert at Cursor › 8157: can insert at cursor from side bar
msedge › tests/smoke/floatingActionButton.spec.ts › sidebar page smoke test › can hide the floating action button
chrome › tests/smoke/pageEditor.spec.ts › page editor smoke test › can open the page editor and connect to an open tab
chrome › tests/telemetry/errors.spec.ts › can report errors to telemetry service

Skipped tests

chrome › tests/regressions/doNotCloseSidebarOnPageEditorSave.spec.ts › #8104: Do not automatically close the sidebar when saving in the Page Editor
msedge › tests/regressions/doNotCloseSidebarOnPageEditorSave.spec.ts › #8104: Do not automatically close the sidebar when saving in the Page Editor
chrome › tests/runtime/googleSheetsIntegration.spec.ts › can activate a google spreadsheet mod with config options
msedge › tests/runtime/googleSheetsIntegration.spec.ts › can activate a google spreadsheet mod with config options
chrome › tests/runtime/insertAtCursor.spec.ts › Insert at Cursor › 8154: can insert at cursor after opening sidebar from selection menu
msedge › tests/runtime/insertAtCursor.spec.ts › Insert at Cursor › 8154: can insert at cursor after opening sidebar from selection menu

Copy link

codecov bot commented Sep 9, 2024

Codecov Report

Attention: Patch coverage is 95.45455% with 1 line in your changes missing coverage. Please review.

Project coverage is 74.73%. Comparing base (8318d74) to head (f82e25a).
Report is 268 commits behind head on main.

Files with missing lines Patch % Lines
src/contentScript/pipelineProtocol/runMapArgs.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9122      +/-   ##
==========================================
+ Coverage   74.24%   74.73%   +0.48%     
==========================================
  Files        1332     1345      +13     
  Lines       40817    41647     +830     
  Branches     7634     7756     +122     
==========================================
+ Hits        30306    31125     +819     
- Misses      10511    10522      +11     

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

}: {
namespace: StateNamespace;
modComponentRef: Except<ModComponentRef, "starterBrickId">;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know what lead to the order change, but if we ever want to enforce order for object keys or react props (that one can be a bit more problematic), there are some lint rules for it:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a functional change - just improving information hierarchy on the order

Copy link

github-actions bot commented Sep 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.

@twschiller twschiller enabled auto-merge (squash) September 9, 2024 14:02
@twschiller twschiller merged commit b3d14f1 into main Sep 9, 2024
23 checks passed
@twschiller twschiller deleted the feature/9105-async-get-set branch September 9, 2024 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants