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

Rename "composeMocks" to better describe what it does #100

Closed
kettanaito opened this issue Apr 15, 2020 · 15 comments · Fixed by #124
Closed

Rename "composeMocks" to better describe what it does #100

kettanaito opened this issue Apr 15, 2020 · 15 comments · Fixed by #124
Assignees
Labels

Comments

@kettanaito
Copy link
Member

Is your feature request related to a problem? Please describe.

I find it confusing that composeMocks() function composes request handlers, and not mocks.

Describe the solution you'd like
I think renaming composeMocks() function to composeHandlers() would provide a more domain-driven name, and won't confuse the users.

- const { start } = composeMocks(...)
+ const { start } = composeHandlers(...)

Benefits of the change

  • Aligned with the internal vocabulary
  • Function does exactly what it's called after

Drawbacks of the change

  • Longer to write
  • Not straight intuitive if you don't know the library's glossary

Alternatives to consider
I was also thinking about simply naming it compose(), but I'm afraid this will clash with the similar functional composition utility that is often called compose(). Also, composeMocks() is not the traditional compose function per-say, as it's logic is specific to MSW.

@kettanaito
Copy link
Member Author

@hehehai, @redraushan, as valuable contributors, what is your opinion on this API change? What consequences do you see this introducing?

@redraushan
Copy link
Contributor

redraushan commented Apr 15, 2020

Hi @kettanaito,
Definitely, composeMocks is not aligned with what it actually does at the end, and compose is quite generic keywords and agree with you that many libraries already have implemented it.

Although, composeHandlers sounder better option that what we already have, however if you ask me I would suggest to look for something which doesn't include compose word at all.
These are a few of them I can think about at the moment:

  1. responseEngine because we are taking requests and giving you response if you start() the engine. You wont get the response if you stop() the engine.
  2. responseFactory if responseEngine sounds too fancy.
  3. workerEngine because under the hood its service worker doing all things.
  4. responseHandler because we are taking requests as it is but controlling response
  5. Or maybe any permutation combination of above mention items.

What do you think @hehehai ?

@kettanaito
Copy link
Member Author

kettanaito commented Apr 15, 2020

Dropping the compose part may be a good idea. I'll try to brainstorm of the names below.

  • createSchema()
  • withMocks()
  • addMocks()

@kettanaito kettanaito changed the title Rename "composeMocks" to "composeHandlers" Rename "composeMocks" to better describe what it does Apr 15, 2020
@redraushan
Copy link
Contributor

@kettanaito, anything finalised if we are keeping composeMocks name as it is?

@kettanaito
Copy link
Member Author

I do love how addMocks() reads and sounds, been thinking of it these two days. I find it crucial we also consider this function in isolation (i.e. close to a React component it's associated with). The name should make sense in that case also.

@redraushan
Copy link
Contributor

@kettanaito addMocks sounds readable however the only thing which is off is the parameter of addMocks are request handlers, but maybe request handlers can be also considered as mocks. I don't know I am pretty bad at naming things :D

@kettanaito
Copy link
Member Author

As MSW will eventually support mocking in non-browser environments, I think that such initializer functions like composeMocks() should clearly describe what are they initializing.

I suggest the following API:

useWorker()

function useWorker(...handlers: RequestHandler[]): WorkerAPI

Uses a Service Worker with the given request handlers.

useServer()

function useServer(...handlers: RequestHandler[]): unknown

Stubs an XHR communication with the given request handlers, thus, mimicking a server.

I'm not found of a word "server" where there are no actual servers, so please feel free to share what name makes more sense to you.

@redraushan
Copy link
Contributor

‘useWorker()’ and ‘useServer()’ sounds clean to me, I think we can move forward with this.

@andreawyss
Copy link
Collaborator

in React ‘use’ is the prefix for React hooks. I propose we use the name ‘run’.

runMockServiceWorker
runMockServer

@kettanaito
Copy link
Member Author

@andreawyss, you're right about the hook prefix. We've had an internal discussion, and that React convention has came up as well.

I like how run can partially replace use in meaning, but in regards to Mock Service Worker functionality, I think it's a little confusing to call runMockWorker() when that function doesn't actually run the worker. The start() function does (so you have granular control over when to start or stop the worker).

I believe such API is misleading:

import { runMockWorker } from 'msw'

// Now I expect that the worker runs after I call this function,
// but in reality it's not.
const { start, stop } = runMockWorker(...)

start()

Perhaps, there's a closer replacement to use in semantical meaning?

@andreawyss
Copy link
Collaborator

andreawyss commented Apr 24, 2020

// Browser usage
import { setupServiceWorker } from 'msw'

const { start, stop } = setupServiceWorker(options)
start()

// Node usage
import { setupMockServer } from 'msw'

setupMockServer(options)

@kettanaito
Copy link
Member Author

kettanaito commented Apr 25, 2020

@andreawyss, that looks lovely!

What do you think if we drop the "Service" part, and it becomes:

// Browser usage
setupMockWorker()

// Node usage
setupMockServer()

I'm also thinking that perhaps it would be a good idea to delegate "starting" the mocked server to the usage surface. That is mainly for two reasons:

  1. Resolving a mock response is an async action. That allows you to dispatch side-effects (such as ctx.fetch() to fetch original response), but also means they need to be awaited before returned as response.

https://github.com/open-draft/msw/blob/8fa7d0e490233138072bc05b4ff0088c81a47466/src/handleRequestWith.ts#L74-L77

This makes the request handler itself asynchronous.

  1. Since attaching a mock server is a side-effect on its own, I think it's common to give control over when it starts and ends in your test suite (beforeAll, afterAll, etc.).
import { setupMockServer } from 'msw'
import handlers from '../shared/handlers'

describe('Test', () => {
  beforeAll(async () => {
    const mock = setupMockServer(handlers)
    // Or whichever method that's suitable in the context
    await mock.open()
  })

  afterAll(async () => {
    await mock.close()
  })
})

@andreawyss
Copy link
Collaborator

andreawyss commented Apr 25, 2020

Great. I like the symmetry of the setup function names.

Also the default response latency (delay) would be about 700 ms for the mock worker and about 5ms for the mock server.

These values can be changed in the options passed to the setup functions.

const mockWorker = setupMockWorker({ handlers, latency: 1000 })

const mockServer = setupMockServer({ handlers, latency: 60 })

Using an option object will allow in the future to add new options as needed.

@kettanaito
Copy link
Member Author

@andreawyss, agree on the options object. Actually, version 0.14.0 already ships with the options to start() being an object, so you can opt-out from logging, for example. Global delay (latency) may also be a viable thing to configure.

@kettanaito
Copy link
Member Author

Thank you everybody for this discussion! I've decided to go with the following options:

  • setupWorker()
  • setupServer()

I'm omitting the "mock" part taking in consideration that you import those functions from a library that does mocking already, and it should be a pre-requisite in understanding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants