addHeader doesn't always work about icedfrisby HOT 12 CLOSED

I now understand the witchcraft.
See "Warning for Deep Clone" in https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign
There's a reference to headers being kept.

from icedfrisby.

I've made the simple change to fix this, but there are now failing unit tests. These test run create and addHeader without any get/post/other verb command, something I hadn't anticipated.

Need to work out how to deal with this. I'd definitely rather preserve this than impose a new requirement that _request is called before a bunch of other stuff can be.

So I've discovered why this issue sometimes doesn't exhibit, so maybe the next step is to work out how to make it permanent.

from icedfrisby.

It's https://github.com/IcedFrisby/IcedFrisby/blob/master/lib/icedfrisby.js#L466
When you've got params with headers, you break the reference that's made between outgoing.headers and current.request.headers

from icedfrisby.

We can make this consistently unmagical with Lodash merge or something, but doing that means we need to grab current.request again at toss() time, at which point we probably wanna stop grabbing it (or so much of it) at _request.

Alternative is to introduce some recursive copying of children that are objects. There's probably some lodash helper for that too.

Sigh. This was a trivial fix yesterday...

from icedfrisby.

Ah hmm…

Even after #101 #100 #105 there is a lot of indirection in the parameters. I prefer something way more direct than what we have. Maybe the best thing would be for me to take a stab at cleaning up the header-handling next?

Do you have a failing unit test?

from icedfrisby.

That's a cracking idea! I'll write the unit test now.

from icedfrisby.

Refactoring anything around the headers at this point involves dealing with the tanglements of globalSetup. Since we've decided to remove that, I'm going to go ahead and do that now. The shared state issue will be fixed along the way.

Will that fit into the changes you're making?

from icedfrisby.

I was only making changes to fix problems after I made changes to my local repo of tests to cope with the globalSetup problem, so they can all be binned.

Here's the failing test:

it('should send all headers when you bootstrap with params', function(){

    frisby.create(this.test.title)
      .baseUri('http://httpbin.org')
      .get('/headers',{headers:{'Abc':'def'}})
      .addHeader('Foo','bar')
      .expectContainsJSON('headers',{
        "Abc": "def"
      })
      .expectContainsJSON('headers',{
        "Foo": "bar"
      })
      .toss()
  })

In my context, the params going into the get/post are coming from a shared variable that was the request section of the globalSetup

from icedfrisby.

Ooh, interesting. Hmm, I bet that will be failing on my branch too.

I'm working through adding a .config() method to replace the settings that globalSetup() offered.

from icedfrisby.

I pushed a branch with some progress. Right now the config() method only works correctly if it's invoked before _request(). Fixing that is going to take some more untangling, so I think it's worth leaving there for now.

I opened #106. There's probably a bit of work left to do… feel free to test and/or push some commits. I'll pick it up again shortly!

from icedfrisby.

Looks like this is on track to be fixed in #106.

from icedfrisby.

This is fixed

from icedfrisby.