Made pretender._handlerFor public

[izelnakri]

Reasoning:

Many complex test suites use this library to mock their backend server, and these tests and route handlers can get very complex. When we write e2e tests it is useful to override route handlers make server respond with error. However in such scenarios it would be also useful to test that user can fix their errors when server goes online/behaves correctly while staying on the same page with the error. Thus a developer would like to override the overridden route with historical route handling logic, instead of overriding with custom logic twice:

test('testing form submit error', function(assert) {
  // Saving previous handler to variable:
  let previousHandler = this.server.pretender.handlerFor('POST', '/users', () => {}).handler;
  
  // mocking the endpoint for error:
  this.server.post(`/users`, () => {
     assert.step('api-call-error');

     return new Response(500, {}, {});
   });

  // click()
   
  // assert error case

  // Override the overridden route with the initial handler, so no more extra mocking needed:
  this.server.post('/users', previousHandler);

  // click()

  // assert success case
});

This change encourages developers to use this pattern, which makes tests behave slightly more like their backend, if their backend is mocked correctly, and makes the test easier to debug since there are less versions of mocking of the same endpoint in the entire test suite.

Additional details:

• In a separate commit, prettier config which matches the source code added to package.json.
• In a separte commit, phantomjs removed since it isn't relevant anymore and breaks yarn install for contributing.

[izelnakri]

Hi @xg-wang , we need this functionality in a large codebase for various use-cases, so mirage can also make it public afterwards, let me know if you have further questions. Otherwise we would like this to be merged.

[xg-wang]

Hi @izelnakri, in your example, this.server.post(${notifierBaseURL}/v1/rules, previousHandler); is it supposed to be this.server.post(/users, previousHandler);?
Looks like you want to toggle the handler's behavior in a test by retrieving the handler for the route and verb combination, which you can do:

1. const originalHandler = server.get(/* ... */);
2. the handler access a variable that can be toggled in the test to change response.

Is the need here about not able to access the originally defined handler therefore you need to read it from within your test?

[izelnakri]

yes 🤦 its is this.server.post('/users', previousHandler); again. Updated the Issue description.

[izelnakri]

Interesting, the calls return the handler? I'll try this out and get back to you! Thank you very much for your response/showing me the alternatives! Might be useful to document this in the README.md

[xg-wang]

@izelnakri no problem! Let me know if that works for you. I can update the README later

[izelnakri]

ok I just found one more issue with your proposed solutions:

1- This is interesting for few test cases I will try. Would be also nice to document it.

2- In a big application with many route handlers your second solution would mean exporting the route handler as ES module function export, currently routes can be registered without separating the handler, and its more readable this way. We already kind of put a symbolic reference by calling the http method and declaring a route string:

this.get('/users') so we should be able to lookup a registry by providing this info I think, thus we dont need to refactor all our route handlers to make them overridable with the same route handler default in our large server mock code

[izelnakri]

Hi @xg-wang , I hope the arguments I made above are convincing for this pull request to be merged, as it is just a removal of the _ in the method name and making it public. Without this small change in a large codebase, I had objections of using a private API, although this shouldnt be a private API based on my reasonings above, including your suggestions. Thus, for the benefit of everyone we should have this method not private.

There was a reason why this method was needed internally in the library in the first place, and very good reasons to be made public.

[xg-wang]

@izelnakri I think we need

• test and documentation to cover what the new public API support, and what is the returned value (route-recognizer result https://github.com/tildeio/route-recognizer/blob/master/README.md#usage)
• https://github.com/pretenderjs/pretender/blob/v3.4.3/src/index.ts#L232-L240 what happens when trying to get from URL / verb never registered, simply creating a new one doesn't look great.
• Also I haven't investigated, what are scenarios matches have more than one entry? https://github.com/pretenderjs/pretender/blob/v3.4.3/src/index.ts#L236
• Can we get better name? It's really returning the route-recognizer matched result.
• What is the last param request supposed to do?

I'm also not sure what is a public way to re-register a route handler for route-recognizer. That said, providing an easy way to retrieve the registered handler you've registered in pretender is fine. The current way to retrieve them from readme is not great const handler = server.handlers[0];

Sorry I do not currently have time for my own thoughts here, but wanted to share with you what I think is needed to make the API public.

To unblock yourself from the objections it looks like you can do it with public APIs

    let registry = pretender.hosts.forURL(url)[verb];
    let matches = registry.recognize(pretender.parseURL(url).fullpath);

    let match = matches ? matches[0] : null;

    return match;

[izelnakri]

Awesome, Ill read the points in detail. Thank you for your detailed answer, and looking into this in depth 👍