Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Regression: cannot extend headers in requestTransform fn #11438

Closed
Narretz opened this issue Mar 26, 2015 · 6 comments
Closed

Regression: cannot extend headers in requestTransform fn #11438

Narretz opened this issue Mar 26, 2015 · 6 comments

Comments

@Narretz
Copy link
Contributor

Narretz commented Mar 26, 2015

Before 5da1256, it used to be possible to do this:

        function requestTransform(data, headers) {
            headers = angular.extend(headers(), {
              'X-MY_HEADER': 'abcd'
            });
          }
          return angular.isObject(data) ? angular.toJson(data) : data;
        }

This is because the headersGetter used to re-use the argument instead of declaring a new one: 5da1256#diff-748e0a1e1a7db3458d5f95d59d7e16c9L71

/cc @pkozlowski-opensource

@Narretz Narretz added this to the 1.4.0-rc.0 / 1.3.17 milestone Mar 26, 2015
@Narretz Narretz changed the title Regression: cannot extend headers in transformData fn Regression: cannot extend headers in requestTransform fn Mar 26, 2015
@pkozlowski-opensource
Copy link
Member

I would argue that this is incorrect usage since the idea behind transformRequest is to transform data to be sent - if I understand its intention correctly. In this sense it is like a filter so it really shouldn't assume that it can have side effects on its arguments....

My vote would be to documenting it as a breaking change.

@pkozlowski-opensource
Copy link
Member

If someone wants to modify headers it should be done via a header being a function.

@Narretz
Copy link
Contributor Author

Narretz commented Mar 26, 2015

I guess it's pretty weird to use it that way, yes.
You could basically change the whole headers object if you wanted to ... you can still to this with an interceptor, so that use case is covered.
An adding headers in the config is obviously the preferred way.

@pkozlowski-opensource
Copy link
Member

Right, so my proposal is this:

  • let's write an explicit tests that shows that we are not allowing this
  • let's update the changelog with a breaking change (I'm on a fence here)

How does it sound @Narretz ?

@Narretz
Copy link
Contributor Author

Narretz commented Mar 26, 2015

We've had breaking change notes for very minor things, so we should definitely do it. I'm not sure a test is needed, but it can't hurt.

@pkozlowski-opensource
Copy link
Member

@Narretz I've sent #11503 to close this one. Mind having a look?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.