Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: refactor helpers #220

Merged
merged 9 commits into from
Oct 28, 2020
Merged

chore: refactor helpers #220

merged 9 commits into from
Oct 28, 2020

Conversation

kalinchernev
Copy link
Contributor

@kalinchernev kalinchernev commented Oct 28, 2020

Reorganizing code by modules related to their purpose, which makes the specification-related utils easier to manage in 1 place.
Other helpers have also been reorganized into combined modules, which makes it easier to mock selected functions within modules.

BREAKING CHANGES

  • The support for deprecated properties has been removed. Please use the plural form. This support for deprecated properties has lasted for 4 years.
  • Upgraded to Erbium (LTS)

@kalinchernev kalinchernev merged commit 391c997 into master Oct 28, 2020
@kalinchernev kalinchernev deleted the chore/refactor-helpers branch October 28, 2020 15:36
@arizonatribe
Copy link

@sibelius
Copy link
Collaborator

what about using Typescript?

@arizonatribe
Copy link

Because of the way people use leading wildcards in their package.json it will cause the docs to not be picked up at all on one machine running 4.3.2 and still work on another one running 4.3.0. This is what happened to us and we wasted an hour of work trying to figure out what was wrong.

It's a good idea to wait until a major release to introduce a change like removing support for the deprecated tags. In the future can you please use proper semantic versioning practices and not introduce a breaking change on a patch release? At the very least you can do a minor release (ideally it would have been better to introduce this breaking change in a major release).

@kalinchernev
Copy link
Contributor Author

Apologies for the dead link, here it is

function getSwaggerSchemaWrongProperties() {

Not sure what you mean with the feedback regarding semantic versioning.
For this particular breaking change, there is a major bump https://www.npmjs.com/package/swagger-jsdoc/v/5.0.0 and a corresponding release https://github.com/Surnet/swagger-jsdoc/releases/tag/v5.0.0 What is not semantic about this?

Because of the way people use leading wildcards in their package.json

You can change this in order to not get major releases before you prepare for them.

@arizonatribe
Copy link

Correct. If I'm not prepared for breaking changes in major release v5 then I can remain on v4 through the use of the proper wildcard.

For that to work it is important that the package author not introduce a breaking change in version 4. That is what we encountered on 4.3.2.

The dropping of support for those deprecated singular tag names should have waited until version 5. But because you introduced them in a "patch" version it makes it so that we receive that breaking change before we are ready for it. And the only way to guard against that now is to lock in to exactly version 4.3.0.

Changes that drop support for certain features - which have worked all throughout an entire major version - are not meant for a patch version (ie, a release which increments only the final digit in the version of a package.json). Package consumers rely on these concepts being observed for semantic versioning to work as intended.

@kalinchernev
Copy link
Contributor Author

@arizonatribe thank you for the elaborate explanation.

The mistake comes from the automated publishing script bumping patch instead of a breaking change. Keywords not detected and thus the lack of correct version.

Apologies once again.

@arizonatribe
Copy link

Okay I understand. Thank you for replying. I appreciate the work you do on this project

@kalinchernev
Copy link
Contributor Author

I thank YOU for reporting the issue! I added warning on the tag here on github with reference to the discussion for the others who might have the same issue. I hope it'll be the less harm than unpublishing the wrong version from npm...

Many thanks and apologies for the time wasted on this!

PS Major bumps will be done manually in the future

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants