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.

Sign up for GitHub

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

Jump to bottom

feat: remove previews #538

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: remove previews #538

wants to merge 2 commits into from

Conversation

wolfy1339
Copy link
Member

@wolfy1339 wolfy1339 commented Jan 3, 2023 •
edited

Followup to octokit/endpoint.js#382 (comment)
See also octokit/endpoint.js#388 octokit/plugin-paginate-rest.js#486
Requires octokit/types.ts#491

Behavior

Before the change?

  • Previews were documented

After the change?

  • Previews are no longer documented

Other information

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 Status: Blocked Some technical or requirement is blocking the issue Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR Type: Breaking change Used to note any change that requires a major version bump labels Jan 3, 2023
@ghost ghost added this to Maintenance in JS Jan 3, 2023
This was referenced Jan 3, 2023
Merged
Merged
@wolfy1339
Copy link
Member Author

Previews still exist for the GraphQL endpoint, this as-is won't work

@oscard0m
Copy link
Member

oscard0m commented Jan 8, 2023 •
edited

Previews still exist for the GraphQL endpoint, this as-is won't work

What do you suggest we should do @wolfy1339 ?

@wolfy1339
Copy link
Member Author

wolfy1339 commented Jan 8, 2023 •
edited

For the types, we can probably do a ternary expression based on the endpoint, to only have previews for GraphQL as it is only one endpoint (/graphql)

For the code itself a simple if statement can work

How does that sound?

@oscard0m
Copy link
Member

For the types, we can probably do a ternary expression based on the endpoint, to only have previews for GraphQL as it is only one endpoint (/graphql)

For the code itself a simple if statement can work

How does that sound?

Sounds good to me.

@wolfy1339 wolfy1339 changed the base branch from main to beta May 21, 2023 22:46
wolfy1339 and others added 2 commits May 21, 2023 18:49
@wolfy1339
ci: stop testing against NodeJS v14, v16 (#561)
11d6862 
* build(package): set minimal node version in engines field to v18
* build: update esbuild options to use Node 18
BREAKING CHANGE: Drop support for NodeJS v14, v16
@wolfy1339
feat: remove previews
ecce98e
Base automatically changed from beta to main July 10, 2023 19:11
@wolfy1339
Copy link
Member Author

Since the underlying packages have been updated to no longer support previews for the REST API, this shouldn't be a breaking change.

I will come back to this once things calm down with the major version bumps

@gr2m
Copy link
Contributor

gr2m commented Jul 11, 2023

if you could add a test to https://github.com/octokit/core.js/blob/main/test/graphql.test.ts with a preview that'd be 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oscard0m oscard0m oscard0m approved these changes

Assignees
No one assigned
Labels
Status: Blocked Some technical or requirement is blocking the issue 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
🧰 Octokit Active
Status: 🛑 Blocked/Awaiting Response
JS
  
Maintenance
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@wolfy1339 @oscard0m @gr2m