apigee-127 / swagger-tools

A Node.js and browser module that provides tooling around Swagger.
MIT License
702 stars 373 forks source link

Lots of changes on a patch release? #537

Closed ruffrey closed 7 years ago

ruffrey commented 7 years ago

Going from 0.10.1 to 0.10.2 results in a lot of changes, including what appears to be a complete rewrite of swagger UI.

It seems far beyond what one would expect in a semver patch release. We just happened to deploy an old project and were very concerned to see a branch new UI.

Is there any possibility of deploying 0.10.3 as a version that is roughly the same as 0.10.1, and then deploying 0.10.2 again as either 1.0.0 or at the minimum 0.11.0?

samuelg commented 7 years ago

Just noticed a new version (released as a patch) includes commits from over a year and a half of work.

whitlockjc commented 7 years ago

Functionally, as in the API, CLI and middleware nothing changes. The swagger-ui bump is as requested by the community. If you want to go back to an older version of swagger-ui, you can download the version the suits your needs and use the built-in feature to override the provided version. I'm all ears but from a semver perspective, nothing changed with the API and the changes should be backward compatible because it was just dependency updates. If you find that the API, CLI or middleware fail to function as they did before, that's a different story of course.

whitlockjc commented 7 years ago

The API, CLI and middleware are heavily tested and no tests required changes. (Of course, "no tests" is not completely accurate, one test swapped from a string equality check to a regex check but it was due to a difference in development tooling and should not affect you.)

whitlockjc commented 7 years ago

Also, please don't think I'm not listening or that I'm not interested in coming up with a decent solution. I'm all ears but I definitely want to understand the scope of "results in a lot of changes" because if it's just swagger-ui, that's a different scenario than API/CLI/middleware functionally changing.

whitlockjc commented 7 years ago

I should had never shipped swagger-ui with this project... But hindsight is 20/20 wouldn't you say?

samuelg commented 7 years ago

My main concern is that the last patch version was in Feb of 2016. A quick glance shows nested controllers are now supported as well as typescript. This feels like a patch bump is not sufficient for the amount of changes that went in, including new features. I would expect a minor bump at the very least.

whitlockjc commented 7 years ago

Nest controllers should had bumped a minor, you're right. I spaced on that one. The TypeScript work has no code impact and is just type data for TypeScript projects. The only functional change was the nested controllers and I forgot that was there, it's been a while. Suggestions?

whitlockjc commented 7 years ago

I can roll back the swagger-ui update and the nested controllers, push a patch release. I can then merge them back in and push a minor release. With NPM not letting me re-publish using the same version, it might require a new patch release unless you can just make the changes on the current patch tag, or replace it. Those are the only to functional changes.

To explain, I do see swagger-ui as just a dependency so updating it when updating other dependencies seems fair, but if it's a new UI I can see why that would be considered "breaking". As for the nested controllers, it's a backward compatible change so if there is no breaking change it could go either way, but since it's a new feature it probably should had been a minor release. Seems like a lot of work but I'd do it for y'all if it meant something to you.

veedeo commented 7 years ago

Since 0.10.2 few our projects are failing with error "no route found - returning default error" so its definitely contains a breaking changes. Ill investigate further when I have more time

samuelg commented 7 years ago

It does seem to me that bug fixes should have gone in 0.10.2 with at the very least nested controllers (possibly swagger-ui as well) in a minor release but due to NPM not allowing a republish and the above "no route found error" I'm not sure what the best course of action is at this point. May have to wait to see what the issue is before going further down this discussion.

ruffrey commented 7 years ago

Thanks for looking into this.

With NPM not letting me re-publish using the same version, it might require a new patch release unless you can just make the changes on the current patch tag, or replace it.

IMO publishing fixes to 0.10.3 would be just fine. Few people will be locked to 0.10.2 yet, and npm would automatically skip 0.10.2 for 0.10.3 in most cases.

whitlockjc commented 7 years ago

@veedeo Please do. That error is not a swagger-tools error so I'm interested in hearing how it's failing. The only thing I could think of if we updated path-to-regexp, assuming this is a failure in swagger-metadata and/or swagger-router matching routes, but we didn't. As mentioned earlier, all swagger-tools tests passed without alteration so there might be some case we don't test for.

whitlockjc commented 7 years ago

@samuelg Yeah, that's what I proposed above. If that's the case, I'll just back out the breaking changes from the patch release as 0.10.3 and release a 0.11.0 from master with all changes (and any necessary fix required by @veedeo).

samuelg commented 7 years ago

Sounds good to me. Thanks for responding so quickly!

smolnikov commented 7 years ago

@whitlockjc there is one breaking change I noticed. This update breaks projects built on a127 for Apigee. Dependencies chain leads to json-refs@3.0.1 which has path.isAbsolute on line 253, and path.isAbsolute is only available since node@0.11.2, but Apigee only supports node@0.10.32 :(

whitlockjc commented 7 years ago

@smolnikov Unfortunately, there are no requirements to keep swagger-tools working on Apigee Edge but I'm completely fine with supporting v0.10.x if possible. One thing you could do in the meantime is to monkey patch path.isAbsolute. (This is a common practice for Apigee Edge customers where Trireme, the JVM-based runtime enabling Node.js on Apigee Edge, works differently than expected.)

whitlockjc commented 7 years ago

I have released swagger-tools@0.10.3 which rolls back the 2 commits related to nested controllers. I kept in all other changes since they are not new features and were bug fixes, or dependency updates. I understand that updating swagger-ui could be seen as a "breaking change" but the more I think about it, swagger-ui is a dependency like any other and thanks to the swagger-ui middleware options, you can easily downgrade your swagger-ui installation. (If you want to know what changes I have to make to swagger-ui to make it "just work" with swagger-tools, diff index.html.orig and index.html.)

As for swagger-tools no longer working on v0.10.x of Node.js, this is really something you should request in json-refs, although that's no guarantee it will be done. I'm closing this but feel free to continue the conversation, I'll gladly listen and participate.

wolph commented 7 years ago

For others (like me) having problems on Ubuntu LTS 14.04, simply installing swagger-tools@0.10.1 works around the issue for the time being.

whitlockjc commented 7 years ago

@WoLpH What problems are you seeing? I cannot fix them unless I know what they are and I don't see you commenting above. I look forward to your response.

wolph commented 7 years ago

@whitlockjc the isAbsolute error:

# swagger-tools validate swagger.json

  error: Object #<Object> has no method 'isAbsolute'

make: *** [codegen] Error 1
whitlockjc commented 7 years ago

swagger-tools supporting v0.10.x doesn't make much sense, especially when the dependencies we use do not support that old of a version. Based on the example you gave, you can still use a newer version of Node.js to run swagger-tools and code against v0.10.x for Apigee Edge. And in Apigee Edge, if you're using swagger-tools API/middleware, all it takes is for you to monkey-patch the path.isAbsolute as suggested.

wolph commented 7 years ago

While I understand your standpoint I would urge you to consider an alternative solution. Ubuntu 14.04 LTS is quite commonly used and has official support till April 2019 so I expect to find many machines in the wild for the time being.

As it stands now on a stock Ubuntu 14.04 machine with a stock install of nodejs your tool doesn't work. Both because of this issue and because Ubuntu calls the binary nodejs instead of node, but that's easily fixed with an alias. Expecting people to monkeypatch code upon installation seems silly imho ;)

whitlockjc commented 7 years ago

Unfortunately, Node.js v0.10.x has been EOL for almost a year:

https://github.com/nodejs/Release#release-schedule1

I understand your situation but I'm not sure how it's possible to keep up with all of the conflicting LTS agreements. And this is even more complex when downstream dependencies come into play.

As for monkey patching, it's a common practice that is very simple to make work. I agree that it could be seen as "silly" and painful but I'm just suggesting a known pattern for working around Node.js version differences/incompatibilities. (In the original mention of path.isAbsolute, they mentioned they were Apigee Edge customers and at Apigee monkey-patching was the suggested approach for working around incompatibilities in their Node.js support, provided by Trireme.)

wolph commented 7 years ago

Yeah, I see the issue... Ubuntu can be a huge pain in the !@#$% with these. Is there perhaps an easy way to detect the issue so at least the error (and fix) is clearer and obvious? It's currently in a works-for-me state but it's likely that more people will run into the issue.