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

feat: remove previews #491

Merged
merged 1 commit into from
Jan 11, 2023
Merged

feat: remove previews #491

merged 1 commit into from
Jan 11, 2023

Conversation

wolfy1339
Copy link
Member

@wolfy1339 wolfy1339 commented Jan 3, 2023

Followup to octokit/endpoint.js#382 (comment)
See also octokit/endpoint.js#388 octokit/core.js#538 octokit/plugin-paginate-rest.js#486


Behavior

Before the change?

  • Previews were documented

After the change?

  • Previews are no longer documented

Other information

  • This PR must be merged before all other ones in the Octokit JS repos

Additional info

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Added the appropriate label for the given change

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

  • Yes (Please add the Type: Breaking change label)
  • No

If Yes, what's the impact:

  • Removal of previews

Pull request type

Please add the corresponding label for change this PR introduces:

  • Bugfix: Type: Bug
  • Feature/model/API additions: Type: Feature
  • Updates to docs or samples: Type: Documentation
  • Dependencies/code cleanup: Type: Maintenance

@wolfy1339 wolfy1339 added Type: Breaking change Used to note any change that requires a major version bump Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR labels Jan 3, 2023
@nickfloyd nickfloyd merged commit cb0835b into main Jan 11, 2023
@nickfloyd nickfloyd deleted the remove-previews branch January 11, 2023 17:59
@octokitbot
Copy link
Collaborator

🎉 This PR is included in version 8.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@wolfy1339
Copy link
Member Author

This unfortunately removes all previews. A follow up is needed to add it from GraphQL

@kfcampbell
Copy link
Member

@wolfy1339 do you think it's worth a revert of this PR in the meantime? I'm not sure I understand the scope of the issue.

@wolfy1339
Copy link
Member Author

I don't think a revert is warranted here.

We can probably handle it in the packages that have GraphQL themselves.

  • @octokit/core
  • @octokit/graphql
  • octokit (octokit.js)

Types would need to be handled there

@nickfloyd
Copy link
Contributor

It looks like this broke plugin-enterprise-server.js as well - I should've taken a closer look and remembered that there are still old supported versions of GHE that have references to previews.

@nickfloyd
Copy link
Contributor

On the upside, GHE 3.2 (which had the majority of the preview implementation is deprecated: https://docs.github.com/en/enterprise-server@3.7/admin/all-releases

I'll need to check 3.3 (set to deprecate next week 01.18.2023) and 3.4 to make sure they are all gone from GHE versions - if that's the case we can just drop support for those versions as well and not rollback.

Thoughts?

@nickfloyd
Copy link
Contributor

Update: Two instances have been left out in the REST API descriptions though they are defunct by v3.3.

@wolfy1339 It looks like we have two options here:

  1. Revert
  2. Clean up the files like this and deprecate v3.2 and v3.3 and move on.

Let me know your thoughts when you get the chance. Thanks!

@wolfy1339
Copy link
Member Author

Sounds good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Type: Breaking change Used to note any change that requires a major version bump Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants