[#804] OPTIONS request mocking support #841

riklaunim · 2016-08-13T12:43:22Z

Initial commit for OPTIONS request mocking. Does not work yet, throwing:

TypeError: this.pretender[verb] is not a function

When options is trying to get mocked.

riklaunim · 2016-08-13T12:43:57Z

addon/route-handler.js

@@ -41,6 +41,8 @@ function createHandler({ verb, schema, serializerOrRegistry, path, rawHandler, o
 handler = new DeleteShorthandHandler(...args);
 } else if (verb === 'head') {
 handler = new HeadShorthandHandler(...args);
+ } else if (verb === 'options') {
+ handler = new GetShorthandHandler(...args);


This will rather require it's own handler.

can we make this the FunctionHandler? I don't think shorthands make sense for options (or do they?)

samselikoff · 2016-08-28T01:11:19Z

Looks like Pretender supports OPTIONS.

Regarding the shorthand handler, I agree it should have its own handler. Should there even be a shorthand for OPTIONS? What would the behavior be?

Perhaps we can start with just adding the verb, and letting users use the function handler, e.g.

this.options('/some/url', () => {
  // normal function handler response
});

yankeeinlondon · 2016-08-31T14:03:01Z

@samselikoff I developed a PR but it looks too similar to this one to add it to the repo. the sad part though is that neither seems to address allowing a passthrough of a OPTIONS & POST pairing.

I'm also worried that maybe there's another part to this. My error:

Seems to me the OPTIONS error isn't the primary error we're getting (at least in my case). It both says it is accepting a passthrough on the POST message (which follows the OPTIONs call) and then errors on the call.

yankeeinlondon · 2016-08-31T14:28:03Z

Ok, my last comment is a bit misleading. I had in earlier testing removed the "options" from the my config.js. Now that it is corrected to read:

this.passthrough('https://www.googleapis.com/identitytoolkit/v3/relyingparty/verifyPassword', ['post', 'head', 'options']);

I'm back to my state from yesterday where it is fails on the "options" verb of the pretender object:
https:/samselikoff/ember-cli-mirage/blob/master/addon/server.js#L179

If I look at the properties hanging off of that Pretender object I still see:

There must be somewhere, that I haven't grokked yet where the options() config needs to be added to Pretender?

yankeeinlondon · 2016-08-31T14:53:43Z

Sorry to be so verbose but ... another update ...

I switched the URL to a shortened version with wildcard characters:

this.passthrough('https://www.googleapis.com/**', ['post', 'head', 'options']);

this DOES work with this PR. 👍

The fully qualified URL (https://www.googleapis.com/identitytoolkit/v3/relyingparty/verifyPassword) should work too but for some reason it does not.

yankeeinlondon · 2016-08-31T14:59:34Z

Any chance we could merge this in? even with the fully qualified URL it is an improvement I think.

riklaunim · 2016-08-31T15:04:05Z

I need to fix the style validation errors and see what else is failing. Either way you can use a git commit as the version in package.json if you just want to use it now :)

git+https:/riklaunim/ember-cli-mirage.git#abae084fe2046648a7b26b31e49df2841c9f90fb

should do the trick.

yankeeinlondon · 2016-08-31T15:07:14Z

yup @riklaunim that's what I'm doing for now. glad you got to this 18 days before me. :) Thanks for the PR.

One last note, working off your code base I noticed that even though my shortened URL works the passthrough call will fail when explicitly asking for 'options'.

yankeeinlondon · 2016-08-31T15:23:56Z

@riklaunim in fact I can't use yours yet as it fails on the same line as it was with base ember-cli-mirage. I had added one piece of defensive code into my clone of your PR:

verbs.forEach(verb => {
  paths.forEach(path => {
    let fullPath = this._getFullPath(path);
    if(this.pretender[verb]) {
      this.pretender[verb](fullPath, this.pretender.passthrough);
    } else {
      console.warn(`Call to a non-existant pretender method: "${verb}". Pretender is: `, this.pretender);
    }
  });
});

Not sure whether this is worth having in the final code once everything is sorted but it does get this to working.

riklaunim · 2016-09-01T09:03:24Z

Well, I was wondering why Pretender doesn't have my options and other changes visible. Turns out the installed dependency isn't the one that is being used (that's from a docker container):

./bower_components/pretender/pretender.js
./node_modules/ember-cli-mirage/node_modules/pretender/pretender.js

node_modules/ember-cli-mirage/node_modules/pretender/pretender.js is used and not the version I install with my project as ./bower_components/pretender/pretender.js

And Pretender seems to need this pretenderjs/pretender#166 to work. When I added that to the used pretender.js file - options do mock:

this.options('/campaigns', function () {
      return .....;
});

yankeeinlondon · 2016-09-01T09:39:40Z

@riklaunim got a bit confused on your npm versus bower Pretender versions comment. I'd expect that Mirage is only using the npm version but I think that's what you're saying. Anyway, glad to see your PR on Pretender as I pretty sure it had to originate from there based on what we were seeing hanging off the Pretender object. Sounds like you've nailed that.

riklaunim · 2016-09-01T09:43:38Z

installing ember-cli-mirage adds pretender to bower dependencies so I expected it will use it from bower_components, or if it wants it in node_modules then it will be in node_modules and not /node_modules/subfolder/node_modules/ -- which is pretty hidden (and it seems the version can't be changed by the package user).

yankeeinlondon · 2016-09-01T09:47:31Z

Oh yes that is confusing. @samselikoff is there a reason for this?

yankeeinlondon · 2016-09-01T10:18:27Z

@riklaunim I have forked your PR and pointed to your Pretender PR in the package.json so I get the full end-to-end effect. Works. I can send you the PR if you want but I'm guessing you'd prefer to keep these two PR separate even though the Mirage PR depends on the Pretender one.

yankeeinlondon · 2016-09-01T10:35:18Z

I can confirm though that a side effect does seem to be introduced. When I use this PR paired with the Pretender PR, I get no errors but Mirage does not use my config/default.js configuration and therefore fixtures and factories are not used.

Use Case 1: PR breaks scenarios
Use Case 2: PR solves options problem

So to be clear, use case 1a:

using published Mirage 0.2.1
remove all passthrough configuration in mirage/config.js ... without which no OPTIONS calls will be made
load an application page who's model ('account') has a fixture associated with it
the GET /accounts request results in the expected 95 rows that come from the fixture
no error, expected outcome

Use case 1b:

Use this PR alongside the Pretender PR
exact same setup as 1a, outcome though is different ... no error, just no results:

Use Case 2a:

using default Mirage 0.2.1
go to a page where POST https://www.googleapis.com is called (and indirectly the OPTIONS call)
add passthrough --this.passthrough('https://www.googleapis.com/**', ['post', 'head', 'options']); -- to mirage/config.js.
immediately fails on any page with the above mentioned "_this.pretender[verb] is not a function" error

Use Case 2b:

using Mirage and Pretender PR's (see link)
No error, network request goes out as expected ... desired behaviour.

riklaunim · 2016-09-01T10:43:23Z

Pretender needs to merge the PR and push a new version, then this can use it and not a github commit (which would be pretty odd for a production release).

yankeeinlondon · 2016-09-01T10:52:32Z

yes that was my point 👍

I just did the fork for my immediate needs of seeing the two PR's together.

yankeeinlondon · 2016-09-01T10:53:16Z

the issue above does need addressing though; it would be a showstopper break for Mirage as it stands

riklaunim · 2016-09-01T11:01:27Z

I use it with a big client panel demo and it works. If the are problems - test should show them too - when the verb problem is solved by newer pretender.

yankeeinlondon · 2016-09-01T11:18:08Z

so you're loading fixtures and/or factories to initialise your state?

riklaunim · 2016-09-01T11:18:54Z

Mostly factories, few custom responses. I have some passthrough in another project, can check it too.

yankeeinlondon · 2016-09-01T11:20:18Z

would you let me know how that comes out? I have to shift to a different project for a day but if you're not seeing this behaviour I'll dig into mine again and see what else could be causing it.

riklaunim · 2016-09-01T11:45:10Z

Tested, works.

samselikoff · 2016-09-03T03:17:52Z

I'd love the tl;dr from your convo & experiments - what should our next steps be here? Are we blocked by the Pretender pr?

riklaunim · 2016-09-03T11:07:34Z

At least some tests won't pass as Pretender.options does not exist. Then there may be some other options tests for fixing. It's also odd that node_modules/ember-cli-migrage/node_modules/.... subfolders shows up with Pretender - and not just the one in bower_components. I had to hand modify the Pretender code there instead of getting it from a git version in bower dependencies.

I'll see about the Pretender additional tests, then see those tests here.

samselikoff · 2016-09-04T11:34:39Z

Awesome, thank you. So we should hold off on this PR until those are sorted out, right?

I appreciate your help :)

riklaunim · 2016-10-04T08:34:15Z

Now fixture tests fail for some reason.

samselikoff · 2016-10-24T13:46:12Z

package.json

@@ -75,7 +75,7 @@
 "exists-sync": "0.0.3",
 "fake-xml-http-request": "^1.4.0",
 "faker": "3.0.1",
- "pretender": "^1.2.0",
+ "pretender": "^1.3.0",


I think we can remove this change from this PR now? Once you do the tests will rerun and we can investigate any errors.

Let's get this puppy merged!

1.2.0 doesn't have OPTIONS support. 1.3.0 is the first one to have it.

right but master is now on 1.4 so if you remove this & rebase we should be set

ah, rebase to master. OK :)

samselikoff · 2016-10-24T14:11:16Z

You have some failures, are your tests passing locally?

riklaunim · 2016-10-24T14:26:29Z

nop, as I commented above - few fixture related tests fail and I don't why.

riklaunim · 2016-12-08T08:30:12Z

Can you help me with this? It seems like even adding options to the list - 14523a7#diff-3da129bcd0b0367d1b65b28e9ecc34c9 - causes the fixtures tests to fail. For some reason visit() return 404 - like if some mock or something stopped working.

samselikoff · 2017-07-11T04:34:52Z

Closing due to inactivity

riklaunim added 3 commits August 4, 2016 14:06

Add options mocking support.

14523a7

Add missing options request entries.

e745afc

Add handler for options verb.

abae084

riklaunim reviewed Aug 13, 2016
View reviewed changes

Fix code style.

e108fc4

acorncom added the needs submitter response label Sep 22, 2016

riklaunim added 2 commits October 4, 2016 10:09

Bump pretender version.

8c63b90

Merge remote-tracking branch 'upstream/master'

7d5a383

samselikoff reviewed Oct 24, 2016

View reviewed changes

Merge remote-tracking branch 'upstream/master'

5c05171

riklaunim added 2 commits December 6, 2016 10:52

Merge remote-tracking branch 'upstream/master'

cba9082

Use FunctionHandler for options.

dc106b9

samselikoff closed this Jul 11, 2017

[#804] OPTIONS request mocking support #841

[#804] OPTIONS request mocking support #841

Conversation

riklaunim commented Aug 13, 2016

riklaunim Aug 13, 2016

Choose a reason for hiding this comment

samselikoff Sep 3, 2016

Choose a reason for hiding this comment

samselikoff commented Aug 28, 2016

yankeeinlondon commented Aug 31, 2016 • edited Loading

yankeeinlondon commented Aug 31, 2016 • edited Loading

yankeeinlondon commented Aug 31, 2016 • edited Loading

yankeeinlondon commented Aug 31, 2016

riklaunim commented Aug 31, 2016

yankeeinlondon commented Aug 31, 2016

yankeeinlondon commented Aug 31, 2016

riklaunim commented Sep 1, 2016

yankeeinlondon commented Sep 1, 2016

riklaunim commented Sep 1, 2016 • edited Loading

yankeeinlondon commented Sep 1, 2016

yankeeinlondon commented Sep 1, 2016

yankeeinlondon commented Sep 1, 2016 • edited Loading

riklaunim commented Sep 1, 2016

yankeeinlondon commented Sep 1, 2016 • edited Loading

yankeeinlondon commented Sep 1, 2016 • edited Loading

riklaunim commented Sep 1, 2016

yankeeinlondon commented Sep 1, 2016

riklaunim commented Sep 1, 2016

yankeeinlondon commented Sep 1, 2016

riklaunim commented Sep 1, 2016

samselikoff commented Sep 3, 2016

riklaunim commented Sep 3, 2016

samselikoff commented Sep 4, 2016 • edited Loading

riklaunim commented Oct 4, 2016

samselikoff Oct 24, 2016

Choose a reason for hiding this comment

riklaunim Oct 24, 2016

Choose a reason for hiding this comment

samselikoff Oct 24, 2016

Choose a reason for hiding this comment

riklaunim Oct 24, 2016

Choose a reason for hiding this comment

samselikoff commented Oct 24, 2016

riklaunim commented Oct 24, 2016

riklaunim commented Dec 8, 2016

samselikoff commented Jul 11, 2017

yankeeinlondon commented Aug 31, 2016 •

edited

Loading

yankeeinlondon commented Aug 31, 2016 •

edited

Loading

yankeeinlondon commented Aug 31, 2016 •

edited

Loading

riklaunim commented Sep 1, 2016 •

edited

Loading

yankeeinlondon commented Sep 1, 2016 •

edited

Loading

yankeeinlondon commented Sep 1, 2016 •

edited

Loading

yankeeinlondon commented Sep 1, 2016 •

edited

Loading

samselikoff commented Sep 4, 2016 •

edited

Loading