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

Added deprecated to enums #2342

Merged
merged 1 commit into from
Dec 3, 2023
Merged

Added deprecated to enums #2342

merged 1 commit into from
Dec 3, 2023

Conversation

imolorhe
Copy link
Collaborator

@imolorhe imolorhe commented Dec 3, 2023

Fixes

#2339
#2338

Checks

  • Ran yarn test-build
  • Updated relevant documentations
  • Updated matching config options in altair-static

Changes proposed in this pull request:

Copy link

github-actions bot commented Dec 3, 2023

Visit the preview URL for this PR (updated for commit 325e044):

https://altair-gql--pr2342-imolorhe-add-depreca-h4uu58ox.web.app

(expires Sun, 10 Dec 2023 14:27:52 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 02d6323d75a99e532a38922862e269d63351a6cf

@imolorhe imolorhe added this pull request to the merge queue Dec 3, 2023
Merged via the queue into master with commit 9c4d139 Dec 3, 2023
11 of 13 checks passed
@imolorhe imolorhe deleted the imolorhe/add-deprecated-enum branch December 3, 2023 15:33
@@ -15,15 +15,15 @@
"@types/supertest": "^2.0.11",
"jest": "29.4.1",
"koa": "^2.13.1",
"koa-send": "^5.0.1",
Copy link
Contributor

@jaydenseric jaydenseric Dec 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was it added to dev dependencies, instead of dependencies? It's used in the published code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that was by mistake. Thanks for spotting it.

Comment on lines -25 to -26
"@koa/router": "~11.0.2",
"koa-send": "^5.0.1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These were semver major changes, but they were published in non semver major version changes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I'm not sure I follow. You mean I should have published a major version since the change is major?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean I should have published a major version since the change is major?

Yes.

Anytime a package makes its peer dependency requirements tighter (require newer versions), it's a breaking change that should be published in a semver major version increment.

Packages that were previously using @koa/router v11 and altair-koa-middleware will now have npm install errors.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I can publish a major version next but I don't think that would fix the issue with those packages, as they would need to update manually.

Copy link
Contributor

@jaydenseric jaydenseric Dec 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit rusty about the correct way to fix an accidentally major change in a minor/patch release, but maybe what you could do is revert the major changes in an other patch release so that way people using altair-koa-middleware v5 are fixed (for projects not using a lock file , which are most open source published packages, CI will just start working for them again, and npm install for them will start working again), and then redo the major changes correctly in a follow-up v6 release.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good

@jaydenseric jaydenseric mentioned this pull request Dec 6, 2023
3 tasks
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.

2 participants