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

fix(context): Refactor context #9371

Merged
merged 19 commits into from
Dec 25, 2023
Merged

fix(context): Refactor context #9371

merged 19 commits into from
Dec 25, 2023

Conversation

Josh-Walker-GM
Copy link
Collaborator

@Josh-Walker-GM Josh-Walker-GM commented Nov 2, 2023

Problem
Context isolation was partially updated in v6 to remove a legacy flag which allowed disabling of context isolation. This unfortunately introduced some bugs for users which could not be further addressed without more potentially breaking changes. These were mainly centred around users who had introduced their own context isolation/handling into non-graphql functions.

Functions (other than graphql) have never had the assurance of context isolation out of the box - even though we allow direct access to the context in a function like we do in other places such as services. This is clear when you look at the results of k6 tests which test context isolation:
image
I do not think this is behaviour we should support. It is a confusing disparity in behaviour between graphql and other functions.

Changes

  1. Moves context code into it's own package @redwoodjs/context. This is to avoid circular dependencies when it lives in either @redwoodjs/api-server or @redwoodjs/graphql-server. It is also a small package with no external dependencies which also helps to avoid bringing in unneeded code should it live in somewhere like @redwoodjs/api - although I am happy to move this into that package should we feel it'll only ever be used in conjunction with it.
  2. Updates the context isolation mocking during user project tests.
  3. Updates the context isolation tests in the test-project fixture.
  4. Updates k6 tests to reference the new context package.
  5. 'Hides' the getAsyncStoreInstance function as a dist import to discourage direct usage by users.
  6. Adds a babel plugin to wrap all functions to ensure context is setup - in the event it has not been isolated at a server request level.

TODO

  1. I want to confirm this plays nicely with users who have been using context in their api side tests.
  2. I want to go through and tidy up any of this code which could do with it. We can likely deduplicate some code but I don't want to step on any of the other ongoing api server refactoring.
  3. I also want to rename away from GlobalContext to something more descriptive like RedwoodAPIContext or similar.

^ The refactoring TODOs might not make it in here.

@Josh-Walker-GM Josh-Walker-GM added release:breaking This PR is a breaking change release:fix This PR is a fix fixture-ok Override the test project fixture check labels Nov 2, 2023
@Josh-Walker-GM Josh-Walker-GM added this to the v7.0.0 milestone Nov 2, 2023
@Josh-Walker-GM Josh-Walker-GM self-assigned this Nov 2, 2023
@Josh-Walker-GM Josh-Walker-GM removed the release:fix This PR is a fix label Nov 2, 2023
@Josh-Walker-GM Josh-Walker-GM changed the title fix(context): Expand API context isolation to cover all functions fix(context): Refactor context and always isolate at the request level Nov 3, 2023
@Josh-Walker-GM Josh-Walker-GM changed the title fix(context): Refactor context and always isolate at the request level fix(context): Refactor context Nov 9, 2023
@Josh-Walker-GM
Copy link
Collaborator Author

Josh-Walker-GM commented Nov 12, 2023

Testing again with k6 tests shows that context works for the following:

yarn rw serve
yarn rw serve api
yarn rw dev
yarn rw-server
yarn rw-server api

The p(90) of the request durations where also pretty much the same before and after the changes added here. Of course this is all local and not results from deployed apps.


How to test?

  1. I generated a test project with the familiar yarn build:test-project and then sync'd over the framework state.
  2. In the tasks/k6-tests directory you'll find two context k6 tests: context_functions and context_graphql. These should be copied over to the test project, the root directory is fine.
  3. You'll also find setups/context_magic_number in the k6 folder. You will want to copy the templates over too. The func.ts to the api/src/functions directory. The benchmark.sdl.ts SDL and benchmarks.ts service should be copied over into the api/src/graphql and a api/src/services/benchmarks respectively. Be careful if you copy these files from outside this current branch as they will have to reference the new @redwoodjs/context package and not the existing @redwoodjs/graphql-server package.
  4. You can now build and serve the api side using all the possible ways to do.
  5. You should now be able to run each k6 test with the following command:
    k6 run context_functions.js -e TEST_HOST=http://localhost:8911
    
    Noting that you may have to adjust the TEST_HOST to fit the particular serve method. E.g. to http://localhost:8910/.redwood/functions instead.
  6. If the context is setup properly you should see the test pass otherwise it will fail. There will also be a summary.json written to the same location as the test files. This will contain the details of the test and may or may not be useful to look into.

This should all be possible with the yarn test:k6 command too but I tested manually just to confirm.

@Josh-Walker-GM Josh-Walker-GM marked this pull request as ready for review November 13, 2023 12:52
@jtoar jtoar modified the milestones: SSR, v7.0.0 Dec 13, 2023
@Josh-Walker-GM Josh-Walker-GM removed the release:breaking This PR is a breaking change label Dec 24, 2023
@Josh-Walker-GM
Copy link
Collaborator Author

@Tobbe and I discussed this PR and the suggestion was that we attempt to make this not breaking. We restore the original files in the graphql-server package so they remain for anyone using them but are marked deprecated.

I can't guarantee it is not breaking for people who have used our context related code within their own app in an attempt to get context working outside graphql. In this case they should be able to fix any issues by removing their custom code and relying on the functionality now being provided by the framework.

packages/graphql-server/src/globalContext.ts Outdated Show resolved Hide resolved
packages/graphql-server/src/globalContext.ts Outdated Show resolved Hide resolved
packages/graphql-server/src/globalContext.ts Outdated Show resolved Hide resolved
packages/graphql-server/src/globalContext.ts Outdated Show resolved Hide resolved
packages/graphql-server/src/globalContextStore.ts Outdated Show resolved Hide resolved
packages/context/README.md Outdated Show resolved Hide resolved
@Tobbe Tobbe modified the milestones: v7.0.0, next-release Dec 25, 2023
@Tobbe Tobbe added the release:fix This PR is a fix label Dec 25, 2023
@Tobbe Tobbe enabled auto-merge (squash) December 25, 2023 16:57
@Tobbe Tobbe merged commit 6a6687b into main Dec 25, 2023
34 of 36 checks passed
@Tobbe Tobbe deleted the jgmw-api/context-fix branch December 25, 2023 17:23
dac09 added a commit to dac09/redwood that referenced this pull request Dec 27, 2023
…redwood into fix/enhance-error-apollo-suspense

* 'fix/enhance-error-apollo-suspense' of github.com:dac09/redwood: (92 commits)
  chore(deps): update dependency @types/yargs to v17.0.32 (redwoodjs#9759)
  Make it easier to find useMatch docs (redwoodjs#9756)
  chore(unit tests): Use side-effect import to fix TS errors (redwoodjs#9754)
  fix(context): Refactor context (redwoodjs#9371)
  docs: Replaced deprecated <Set private> with PrivateSet within router.md (redwoodjs#9749)
  add TS support for storybook preview tsx config extension (redwoodjs#9309)
  fix(studio): Fix windows path issues (redwoodjs#9752)
  redwoodjs#9620: Update studio to support variable components (Mailer) (redwoodjs#9639)
  chore(tasks): Add comparison view to nmHoisting visualisation (redwoodjs#9751)
  chore(cli): make fs modules used in the CLI consistent (redwoodjs#9746)
  chore(tooling): Make sure console boxen print on a new line
  chore(CI): fix publish release candidate
  feat(CLI): add check node version middleware, rm `.nvmrc`, yarn engines (redwoodjs#9728)
  docs: added some clarification on serverless functions getting executed in a non-serverless environment (redwoodjs#9742)
  Fix sshExec() errors not displaying (redwoodjs#9743)
  chore(tooling): Add missing word in release tooling output
  Update Metadata docs (redwoodjs#9744)
  chore(CI): update test project fixture and CRWA for deploy target CI repo (redwoodjs#9730)
  chore(tooling): add script for getting nested dependency data (redwoodjs#9734)
  Trusted Documents docs: Proofreading corrections (redwoodjs#9737)
  ...
dac09 added a commit to dac09/redwood that referenced this pull request Dec 27, 2023
…ath-aliases

* 'main' of github.com:redwoodjs/redwood: (92 commits)
  chore(deps): update dependency @types/yargs to v17.0.32 (redwoodjs#9759)
  Make it easier to find useMatch docs (redwoodjs#9756)
  chore(unit tests): Use side-effect import to fix TS errors (redwoodjs#9754)
  fix(context): Refactor context (redwoodjs#9371)
  docs: Replaced deprecated <Set private> with PrivateSet within router.md (redwoodjs#9749)
  add TS support for storybook preview tsx config extension (redwoodjs#9309)
  fix(studio): Fix windows path issues (redwoodjs#9752)
  redwoodjs#9620: Update studio to support variable components (Mailer) (redwoodjs#9639)
  chore(tasks): Add comparison view to nmHoisting visualisation (redwoodjs#9751)
  chore(cli): make fs modules used in the CLI consistent (redwoodjs#9746)
  chore(tooling): Make sure console boxen print on a new line
  chore(CI): fix publish release candidate
  feat(CLI): add check node version middleware, rm `.nvmrc`, yarn engines (redwoodjs#9728)
  docs: added some clarification on serverless functions getting executed in a non-serverless environment (redwoodjs#9742)
  Fix sshExec() errors not displaying (redwoodjs#9743)
  chore(tooling): Add missing word in release tooling output
  Update Metadata docs (redwoodjs#9744)
  chore(CI): update test project fixture and CRWA for deploy target CI repo (redwoodjs#9730)
  chore(tooling): add script for getting nested dependency data (redwoodjs#9734)
  Trusted Documents docs: Proofreading corrections (redwoodjs#9737)
  ...
dac09 added a commit to dac09/redwood that referenced this pull request Dec 28, 2023
…p-prebuild

* 'main' of github.com:redwoodjs/redwood: (1608 commits)
  Docker: Update to work with corepack and yarn v4 (redwoodjs#9764)
  [RFC]: useRoutePaths (redwoodjs#9755)
  Adds a note about the two commands you will use with your schema to the top of the schema file (redwoodjs#8589)
  docs: Supertokens.md: Fix typo (redwoodjs#9765)
  Fix supertokens docs & integration issues (redwoodjs#9757)
  fix(apollo): Enhance error differently for Suspense Cells (redwoodjs#9640)
  SSR smoke-test: Use <Metadata /> (redwoodjs#9763)
  chore(deps): update dependency @types/qs to v6.9.11 (redwoodjs#9761)
  chore(ci): Better error handling in detectChanges.mjs (redwoodjs#9762)
  fix(path-alias): Fix aliasing of paths using ts/jsconfig (redwoodjs#9574)
  chore(deps): update dependency @types/yargs to v17.0.32 (redwoodjs#9759)
  Make it easier to find useMatch docs (redwoodjs#9756)
  chore(unit tests): Use side-effect import to fix TS errors (redwoodjs#9754)
  fix(context): Refactor context (redwoodjs#9371)
  docs: Replaced deprecated <Set private> with PrivateSet within router.md (redwoodjs#9749)
  add TS support for storybook preview tsx config extension (redwoodjs#9309)
  fix(studio): Fix windows path issues (redwoodjs#9752)
  chore(tasks): Add comparison view to nmHoisting visualisation (redwoodjs#9751)
  chore(cli): make fs modules used in the CLI consistent (redwoodjs#9746)
  ...
Tobbe added a commit that referenced this pull request Jan 1, 2024
Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
@jtoar jtoar modified the milestones: next-release, v7.0.0 Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixture-ok Override the test project fixture check release:fix This PR is a fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants