search

cyu / rack-cors

Rack Middleware for handling Cross-Origin Resource Sharing (CORS), which makes cross-origin AJAX possible.
MIT License
3.26k stars 263 forks source link

Breaking API change from 1.1.0 to 1.1.1 #201

Closed MXfive closed 4 years ago

MXfive commented 4 years ago

@cyu It seems breaking changes in patch versions is a theme of this repo. (#195 )

We updated from 1.1.0 to 1.1.1 (this PR #196) whilst maintaining the following config:

allow do
      origins  "*"
      resource "/resource/*", headers: :any, methods: :get
      resource "/resource", headers: :any, methods: [:post, :options]
end

This broke all CORS POST requests to our this resource on our API as the first resource is matched and the second is not tried, which unfortunately for us had sizable business impact, breaking a lot of our customers implementations with us before we realized what was going on.

A breaking API change like this constitutes a new major version and upgrade documentation. We had expected the largest and defacto CORS library for Ruby/Rack/Rails with 10s of millions of downloads to have at least some thought and care about changes. However it turns out this is just a one man show where changes are made without review by others, nor are there suitable integration tests to catch API breaking changes like this.

cyu commented 4 years ago

@MXfive thank you for your comments, and I’m sorry that this had a negative impact on your business.

You’re right, this is, up to this point, a one man show, and I believe me when I say I gain no benefit from releasing breaking changes. Unfortunately, this change was made to fix that other breaking you referred to, which has brought me to the realization that any bug fix I make at this point automatically constitute at least a minor if not major version update. From this point forward I’ll provide a means for people to review future code changes before an official release. I would welcome any opinions on what this would look like. I also would welcome any PRs that would help make this suite of tests complete.

As for this particular break — I’m inclined to let this be. This I believe is the correct implementation. The mistake was making this a patch version instead of at least a minor version. Unfortunately, there isn’t a solution for this once this has been made — yanking this version also has consequences that would break other people (which I have also been lectured on before).

MXfive commented 4 years ago

@cyu Thank you for such a level headed response after my quite emotional post that followed me battling production for 2 days straight (we had another unrelated issue at the same time as this which made it much harder to diagnose.). I agree there is nothing that can be done about this version now, especially given how old it is already.

I now fully appreciate the importance of this gem, especially as it does not seem there are any other public options available that I can find for Ruby/Rack. If you'd accept I am happy to offer my help in maintaining this gem with you. I have started with a PR to refactor the classes out into their own files and applied rubocop #202.

I am going to look more deeply at the test suite this weekend and see if there is anything I can do to improve it. I'm also happy to help review any future PRs or issues if you are interested in getting a second set of eyes on things, please let me know.