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 logic #388

Merged
merged 14 commits into from
Jun 13, 2023
Merged

feat: remove previews logic #388

merged 14 commits into from
Jun 13, 2023

Conversation

wolfy1339
Copy link
Member

@wolfy1339 wolfy1339 commented Jan 3, 2023

Followup to #382 (comment)
Closes #382


Behavior

Before the change?

  • One could use previews, and they are supported and documented

After the change?

  • Previews are removed, and no longer documented

Other information

  • Wait until GHES 3.3 is EOL
  • This PR should NOT get merged until this change is made in @octokit/types, and a release is made

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:

  • N/A

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
@wolfy1339 wolfy1339 changed the base branch from main to beta May 22, 2023 15:02
@wolfy1339 wolfy1339 marked this pull request as draft May 22, 2023 15:03
if (url === "/graphql") {
if (options.mediaType.previews!.length) {
const previewsFromAcceptHeader =
headers.accept.match(/[\w-]+(?=-preview)/g) || ([] as string[]);

Check failure

Code scanning / CodeQL

Polynomial regular expression used on uncontrolled data

This [regular expression](1) that depends on [library input](2) may run slow on strings with many repetitions of '-'.
src/merge.ts Outdated Show resolved Hide resolved
@wolfy1339
Copy link
Member Author

Requires update to @octokit/types to be released

@wolfy1339
Copy link
Member Author

wolfy1339 commented May 26, 2023

I'm going to need a hand with some pointers.
Something is overriding headers.accept from the time it gets returned in the parse() function to when it reaches the test

The parse() function returns this object at the very end:

{
  method: 'POST',
  url: 'https://api.github.com//graphql',
  headers: {
    accept: 'application/vnd.github.package-deletes-preview.json,application/vnd.github.flash-preview-preview.json',
    'user-agent': 'myApp v1.2.3+test'
  }
}

However, in the test that calls endpoint.parse() it receives the following for headers.accept: "application/json"

@wolfy1339
Copy link
Member Author

@gr2m Can you take a look

@gr2m
Copy link
Contributor

gr2m commented Jun 13, 2023

Something is overriding headers.accept

We are meddling with the accept header based on the mediaType settings (previews, format), so I assume it's related to that?

Sorry I can't have a look myself right now, maybe tomorrow after work

@wolfy1339
Copy link
Member Author

I removed the test that I was having issues with. The coverage is still 100%

@wolfy1339 wolfy1339 marked this pull request as ready for review June 13, 2023 21:53
@wolfy1339 wolfy1339 removed the Status: Blocked Some technical or requirement is blocking the issue label Jun 13, 2023
@wolfy1339 wolfy1339 merged commit c0dd00a into beta Jun 13, 2023
@wolfy1339 wolfy1339 deleted the remove-previews branch June 13, 2023 22:44
@github-actions
Copy link

🎉 This PR is included in version 8.0.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released on @beta 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.

3 participants