fix: add "ctx.field()" for GraphQL responses #1257

Poivey · 2022-05-27T10:24:19Z

Closes Support custom fields on a mocked GraphQL response #1200

As discussed in the issues, this PR adds a field(fieldName, fieldValue) method to graphQL response context.

fieldName is typed in a way to prevent using errors, data, extensions and <empty string> as a value. Is then any other value we should forbid ?
fieldName is also validated at runtime level.

@kentcdodds In the issue you mentioned invariant as a solution to make sure this field is valid, but I am not sure how it could be use here as we need to check for specific values, not if the value is undefined. Could you tell me how you thought this coule be used here ?

TODO

Update the documentation, new page : https://mswjs.io/docs/api/context/field

src/context/field.ts

kettanaito · 2022-05-30T13:09:08Z

src/context/field.ts

+}
+
+function assertFieldNameIsValid(fieldName: string) {
+  if (forbiddenFieldNamesList.includes(fieldName as ForbiddenFieldNames)) {


Consider using invariant here:

import { devUtils } from '../utils/internals/devUtils' invariant( fieldName !== '', devUtils.formatMessage('Failed to set a custom field on a GraphQL response: field name cannot be empty.') ) invariant( fieldName !== 'data', devUtils.formatMessage('Failed to set a custom "%s" field on a mocked GraphQL response: forbidden field name. Did you mean to call "ctx.data()" instead?', fieldName) ) invariant( fieldName !== 'errors', devUtils.formatMessage('Failed to set a custom "%s" field on a mocked GraphQL response: forbidden field name. Did you mean to call "ctx.errors()" instead?', fieldName) )

I'd also drop the forbidden names array and tackle each scenario individually. It may seem repetitive but we gain more contextual error messages for the developer this way.

What do you think?

Thank you for the examples, I wasn't sure about using invariant when checking for a specific value. I see the value of more explicit error message, I will update the PR.

I think it's really cool that, being a dev tool, MSW can be as explicit about error messages and error behaviors as it wants. Yeah, I'd generally recommend being detailed when it comes to error messages, and if we structure each individual name check separately, we can produce a separate message for each failed state.

✅
I made these changes.

Do you think we could change errors messages generation a bit to reduce duplication ? Or is it better to have the message in the code without formatting to make the search in the code easier ?

It could be something like this

invariant( fieldName !== 'data' && fieldName !== 'errors' && fieldName !== 'extensions', devUtils.formatMessage( 'Failed to set a custom "%s" field on a mocked GraphQL response: forbidden field name. Did you mean to call "ctx.%s()" instead?', fieldName, fieldName, ), )

What bothers me with the current solution is that we use string formatting to create the error message, but we could either:

use it more to reduce duplication (like in the example above)

or not use it a all and keep the same number of lines of code (we use formatting in cases where we already know the value of fieldName). Instead have string without formatting to make search in the code easier.

Do you think we should change it for one of these propositions or keep it as it is ?

I don't think this is worth investing our time at the moment. Repetition is not inherently bad, and I'd much more prefer to see

invariant(a, messageA) invariant(b, messageB) invariant(c, messageC)

Even if it means all messages are almost the same. Yes, you're right, it improves discoverability of the code but also allows for custom cases without having to extend the abstraction. If we decide to give a better message for the b case then we just do that. We know a and c are not affected.

Ok ! So we keep it as it is ?

kettanaito · 2022-05-30T13:09:41Z

src/handlers/GraphQLHandler.ts

@@ -48,6 +50,7 @@ export const graphqlContext: GraphQLContext<any> = {
  extensions,
  errors,
  cookie,
+  field: field,


You can use shorthand assignment here!

Suggested change

field: field,

field,

kettanaito

Hey, @Poivey! Thank you for preparing this, an outstanding work!

I've left a few suggestions, would love to know what you think.

kettanaito · 2022-05-30T13:12:22Z

I've renamed the pull request to be a fix: because we're pre-1.0, and this does not need to bump the minor version—it's a new feature.

codesandbox-ci · 2022-06-02T13:48:01Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit eb1d59e:

Sandbox	Source
MSW React	Configuration

fix mswjs#1200

…eFieldName

… forbiden names values

Poivey · 2022-06-02T14:02:36Z

There was a mistake in the forbidden values for field name of the initial commit. It is now corrected (I wrote exceptions instead of extensions 🤦)

kettanaito · 2022-06-03T11:41:05Z

src/context/field.ts

+  }
+}
+
+function validateFieldName(fieldName: string) {


We should export this function and unit test it.

Also, let's include an explicit return type:

Suggested change

function validateFieldName(fieldName: string) {

function validateFieldName(fieldName: string): void {

kettanaito · 2022-06-03T11:43:59Z

src/context/field.test.ts

+  )
+})
+
+test.each(['data', 'errors', 'extensions'])(


This is absolutely biased but I don't like any logic around test execution. It always makes tests smarter than they should be. If a test is repetitive, then I recommend tackling that on the test setup level but not test orchestration/definition level.

it('throws an error when using forbidden field names', async () => { const tryWithFieldName = (fieldName: string) => { return response(ctx.field(fieldName, 'value')) } expect(() => tryWithFieldName('data')).toThrow('...') expect(() => tryWithFieldName('extensions')).toThrow('...') })

A single test case because input validation is a single piece of logic (even though we validate against multiple forbidden names—it's fine; each name does not equal new behavior).

Clear assertions.

Repetition is abstracted into tryWithFieldName, which is both scoped and readable.

kettanaito · 2022-06-03T11:44:34Z

src/context/field.test.ts

+      field: { errorCode: 'value' },
+      errors: [
+        {
+          message: 'exceeds the limit of awesomeness',


kettanaito · 2022-06-07T10:05:03Z

I've pushed a few polish items, the feature should be ready to go! 🎉

kettanaito · 2022-06-07T11:02:32Z

@Poivey, I'm so thankful for your contributions. You're the best 🥳

Poivey · 2022-06-07T13:50:52Z

@Poivey, I'm so thankful for your contributions. You're the best 🥳

Happy to contribute ! I planned to do the last changes later this week but it's great if you found the time to update it yourself :)

kettanaito · 2022-06-07T23:15:55Z

Released: v0.42.1 🎉

This has been released in v0.42.1!

📄 Release notes
📦 npm package

Make sure to always update to the latest version (npm i msw@latest) to get the newest features and bug fixes.

Predictable release automation by @ossjs/release.

kettanaito reviewed May 30, 2022

View reviewed changes

src/context/field.ts Outdated Show resolved Hide resolved

kettanaito reviewed May 30, 2022

View reviewed changes

kettanaito requested changes May 30, 2022

View reviewed changes

kettanaito changed the title ~~feat: add field() method to graphQL response context~~ fix: add "ctx.field()" for GraphQL responses May 30, 2022

Poivey added 4 commits June 2, 2022 15:49

feat: add field() method to graphQL response context

6fe7312

fix mswjs#1200

style(src/context/field.ts): rename assertFieldNameIsValid to validat…

520331d

…eFieldName

style(src/handlers/graphqlhandler.ts): use shorthand assignemnt

b21d091

refactor(ctx.field()): update filename validation exceptions, correct…

24d20d6

… forbiden names values

Poivey force-pushed the feature/field-graphQL-ctx-method branch from acf5644 to 24d20d6 Compare June 2, 2022 13:49

kettanaito reviewed Jun 3, 2022

View reviewed changes

kettanaito added 2 commits June 7, 2022 12:03

test(field): expand error unit tests

ee9d469

chore(field): add whitespace between name validations

eb1d59e

kettanaito approved these changes Jun 7, 2022

View reviewed changes

kettanaito merged commit 442f48d into mswjs:main Jun 7, 2022

jtoar mentioned this pull request Jun 10, 2022

fix(deps): update dependency msw to v0.42.1 redwoodjs/redwood#5625

Closed

1 task

cadenzah mentioned this pull request Apr 10, 2023

document for ctx.field() has to be added mswjs/mswjs.io#230

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add "ctx.field()" for GraphQL responses #1257

fix: add "ctx.field()" for GraphQL responses #1257

Poivey commented May 27, 2022 •

edited by kettanaito

kettanaito May 30, 2022

Poivey May 30, 2022 •

edited

kettanaito May 30, 2022

Poivey Jun 2, 2022 •

edited

kettanaito Jun 3, 2022

Poivey Jun 3, 2022

kettanaito May 30, 2022

Poivey Jun 2, 2022

kettanaito left a comment

kettanaito commented May 30, 2022

codesandbox-ci bot commented Jun 2, 2022 •

edited

Poivey commented Jun 2, 2022

kettanaito Jun 3, 2022

kettanaito Jun 3, 2022

kettanaito Jun 3, 2022

kettanaito commented Jun 7, 2022

kettanaito commented Jun 7, 2022

Poivey commented Jun 7, 2022 •

edited

kettanaito commented Jun 7, 2022

	function validateFieldName(fieldName: string) {
	function validateFieldName(fieldName: string): void {

fix: add "ctx.field()" for GraphQL responses #1257

fix: add "ctx.field()" for GraphQL responses #1257

Conversation

Poivey commented May 27, 2022 • edited by kettanaito

TODO

Choose a reason for hiding this comment

Poivey May 30, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Poivey Jun 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kettanaito left a comment

Choose a reason for hiding this comment

kettanaito commented May 30, 2022

codesandbox-ci bot commented Jun 2, 2022 • edited

Poivey commented Jun 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kettanaito commented Jun 7, 2022

kettanaito commented Jun 7, 2022

Poivey commented Jun 7, 2022 • edited

kettanaito commented Jun 7, 2022

Released: v0.42.1 🎉

Poivey commented May 27, 2022 •

edited by kettanaito

Poivey May 30, 2022 •

edited

Poivey Jun 2, 2022 •

edited

codesandbox-ci bot commented Jun 2, 2022 •

edited

Poivey commented Jun 7, 2022 •

edited