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(graphql): ability to update/query for appData #19082

Merged
merged 18 commits into from Nov 29, 2021

Conversation

lmiller1990
Copy link
Contributor

@lmiller1990 lmiller1990 commented Nov 24, 2021

Make the GraphQL interface for access/updating local preferences more generic.

# query
{
  localSettings {
    preferences {
      isSpecsListOpen
    }
  }
}

# update
mutation M {
  # value is JSON.stringify({ k1: value1, k1: value2, ... })
  setPreferences(value: "{ \"isSpecsListOpen\": true }")
}

Typing the setPreferences mutation might be good but it seems a bit complex, and might not make sense since a lot of the fields we cache will either go away, or change, so I just set it to take an object which is JSON.stringified. This is how the current save state API works.

It also improves the types for local data. I hope to make these more strict as we go.

image

I also added a missing feature which is "open config in editor" since we needed that and it will use a lot of this functionality (eg, we need to set the correct preferred editor if they don't have one). Here is a demo (shows that the localSettings feature is working correctly, too):

democonfig.mov

@lmiller1990 lmiller1990 requested a review from a team as a code owner November 24, 2021 04:54
@lmiller1990 lmiller1990 requested review from jennifer-shehane and removed request for a team November 24, 2021 04:54
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Nov 24, 2021

Thanks for taking the time to open a PR!

@lmiller1990 lmiller1990 changed the title Lmiller1990/saved state graphql feat(graphql): ability to update/query for appData Nov 24, 2021
@lmiller1990 lmiller1990 removed the request for review from jennifer-shehane November 24, 2021 04:55
@cypress
Copy link

cypress bot commented Nov 24, 2021



Test summary

18145 0 202 0Flakiness 5


Run details

Project cypress
Status Passed
Commit be5e79a
Started Nov 29, 2021 12:38 PM
Ended Nov 29, 2021 12:50 PM
Duration 12:04 💡
OS Linux Debian - 10.10
Browser Multiple

View run in Cypress Dashboard ➡️


Flakiness

commands/net_stubbing_spec.ts Flakiness
1 network stubbing > intercepting request > can delay and throttle a StaticResponse
2 network stubbing > waiting and aliasing > can timeout waiting on a single response using "alias.response"
3 network stubbing > waiting and aliasing > can timeout waiting on a single request using "alias.request"
cypress/proxy-logging-spec.ts Flakiness
1 Proxy Logging > request logging > xhr log has response body/status code
2 ... > works with forceNetworkError

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

Copy link
Contributor

@marktnoonan marktnoonan left a comment

Choose a reason for hiding this comment

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

I think one change is needed for the default values for the preference to survive the initialization, otherwise this works great.

type: 'Boolean',
args: {
// Stringify JSON, eg JSON.stringify({ firstTimeOpening: Date.now() })
value: nonNull(stringArg()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a description for this mutation? And it's not touched in this PR, but we should update the description of the localSettings field as well since it is still referencing just editors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, good catch

@@ -0,0 +1,70 @@
import type { Editor } from '.'

export const defaultPreferences: AllowedState = {
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like these defaults will always be overwritten by the call to actions.localSettings.refreshLocalSettings that happens in initializeData and so don't end up available in what is exposed via qgl.

Should refreshLocalSettings be updated to merge the preferences it finds on the device with the defaults set here, instead of fully replacing the preferences? Unless there's a different purpose for these defaults that I'm not seeing.

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 point... updated to set defaults in the same place as we load the settings. Local preferences will override defaults.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@marktnoonan marktnoonan left a comment

Choose a reason for hiding this comment

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

Looks good, I pushed 1 commit for an out of date description.

@lmiller1990 lmiller1990 merged commit cc3be10 into 10.0-release Nov 29, 2021
@lmiller1990 lmiller1990 deleted the lmiller1990/saved-state-graphql branch November 29, 2021 13:04
tgriesser added a commit that referenced this pull request Nov 29, 2021
* 10.0-release:
  feat(graphql): ability to update/query for appData (#19082)
  fix system test
  fix failing tests due to merge
  resolve conflicts
  test: node_modules installs for system-tests, other improvements (#18574)
  update yarn.lock
  chore(deps): update dependency semantic-release to v17.2.3 [security] (#19022)
  chore: remove flaky ci jobs for main builds (#19071)
  chore(contributing): clarify PULL_REQUEST_TEMPLATE (#19068)
  fix: the shadow root container element is ignored when clicking an element in it. (#18908)
  'Fix' flaky redirect test (#19042)
  release 9.1.0 [skip ci]
  fix: Allow 'this' to be used in overridden commands (#18899)
  fix(react): link to rerender example (#19020)
  chore(deps): update dependency aws-sdk to v2.814.0 [security] (#18948)
  fix: test config overrides leak for .only execution (#18961)
  feat: Set CYPRESS=true as env var in child processes where Cypress runs user code in Node (#18981)
  fix: Restore broken gif (#18987)
  chore: release @cypress/vite-dev-server-v2.2.1
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.

None yet

2 participants