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

fix: non-working hiddenClients configuration option #1387

Merged
merged 10 commits into from May 14, 2024
Merged

fix: non-working hiddenClients configuration option #1387

merged 10 commits into from May 14, 2024

Conversation

HelgeSverre
Copy link
Contributor

Problem
Because the "allTargets()..."-code is defined outside of the usehttpClients() hook, it will only run once (which was probably done due to performance benefits), however this breaks this feature as only the DEFAULT_EXCLUDED_CLIENTS will be applied the first time this runs, and the next time this is meant to run to filter the targets and clients based on the provided user configuration, it is not updated, aka, doesnt work, this is references in #1250.

Solution

This commit fixes the bug, and retains the performance benefit by caching the target list using watchEffect to only update/recompute the target list when the configuration changes.

Further improvements

I'm working on an additional fix, that will add the ability to specify clients in specific targets to ignore/include, since the behaviour right now will hide all the "curl"-s instead of only hiding the "php"-curl if you wanted to do so.

Still WIP, but feedback and pointers welcome, struggling a bit with TypeScript errors, even though running the embedding demo with this config works in the browser:

const configuration = reactive<ReferenceConfiguration>({
  proxy: import.meta.env.VITE_REQUEST_PROXY_URL,
  isEditable: false,
  spec: { content },
  hiddenClients: {
    php: true, // Example: Exclude all PHP clients
    python: true, // Example: Exclude all Python clients
    c: ['libcurl'],
    node: ['fetch'], // We expect the (browser) javascript fetch client to still be available
    javascript: ['jquery', 'xhr'], // Exclude specific JavaScript clients
  },
})

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Apr 5, 2024 •
edited

⚠️ No Changeset found

Latest commit: cfda8c8

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@marclave marclave marked this pull request as ready for review April 14, 2024 00:13
@marclave marclave self-requested a review as a code owner April 14, 2024 00:13
@marclave marclave changed the title Fix non-working hiddenClients configuration option fix: non-working hiddenClients configuration option Apr 14, 2024
@hanspagel hanspagel self-requested a review April 26, 2024 13:55
@hanspagel hanspagel self-assigned this Apr 30, 2024
@hanspagel
Copy link
Member

Thanks for the awesome PR! The type check seems to fail, I’ll look into that. :)

hanspagel
hanspagel approved these changes Apr 30, 2024
View reviewed changes
Copy link
Member

@hanspagel hanspagel left a comment

Choose a reason for hiding this comment

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

Looks 🌟 awesome 🌟 to me!

amritk
amritk approved these changes May 3, 2024
View reviewed changes
Copy link
Member

@amritk amritk left a comment

Choose a reason for hiding this comment

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

Awesome PR! I left a few comments but mostly nit picks and nothing major. Overall its great!

packages/api-reference/src/components/Content/ClientLibraries/ClientSelector.vue Outdated Show resolved Hide resolved
packages/api-reference/src/hooks/useHttpClients.ts Show resolved Hide resolved
packages/api-reference/src/hooks/useHttpClients.ts Outdated Show resolved Hide resolved
@hanspagel
Copy link
Member

I’ll take care of @amritk’s feedback. ✌️

@hanspagel hanspagel force-pushed the main branch 2 times, most recently from da0903a to 6e26d27 Compare May 6, 2024 12:43
@hanspagel hanspagel requested a review from amritk May 6, 2024 12:45
@hanspagel
Copy link
Member

@amritk Would you mind to do a 2nd review? 🙌

amritk
amritk approved these changes May 6, 2024
View reviewed changes
Copy link
Member

@amritk amritk left a comment

Choose a reason for hiding this comment

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

Nice, looks clean 👍🏾

@hanspagel hanspagel assigned hanspagel and unassigned hanspagel May 7, 2024
HelgeSverre and others added 10 commits May 13, 2024 12:51
@HelgeSverre @hanspagel
fix: Because the "allTargets()..."-code is defined outside of the use…
058b748 
…httpClients() hook, it will only run once (which was probably done due to performance benefits), however this breaks this feature as only the DEFAULT_EXCLUDED_CLIENTS will be applied the first time this runs, and the next time this is meant to run to filter the targets and clients based on the provided user configuration, it is not updated, aka, doesnt work, this is references in scalar#1250.

This commit fixes the bug, and retains the performance benefit by caching the target list using watchEffect to only update/recompute the target list when the configuration changes.
Next i will implement the inverse of this, and add the ability to specify clients in specific targets to ignore/include, as currently, if you hide "curl", you hide both the curl CLI tool and the php "curl" client, which is probably not intentional.
@HelgeSverre @hanspagel
feat: allow using an object to specify clients within specific target…
0d00618 
…s to be hidden.

Example:
{
    php: true, // Exclude all PHP clients
    python: true, //  Exclude all Python clients
    node: ['fetch'], // We expect the "fetch" client to still be available in the "javascript" target.
  }
@hanspagel
chore: add tests and fix TS issues
d9ebab2
@hanspagel
chore: disable unirest client by default
2e1fa44
@hanspagel
fix: unirest is not hidden by default
b73fd91
@hanspagel
fix: list of clients still show clients even if they are hidden
8ed9a5f
@hanspagel
fix: add undici again
a472232
@hanspagel
fix: setExcludedClients has wrong type
c694355
@hanspagel
refactor: move to computed, use flatMap instead of filter + map
009e11c
@hanspagel
refactor: don’t use return for a single return
cfda8c8
@hanspagel
Copy link
Member

@marclave Final review? Let me know if it needs further changes. 👍

marclave
marclave approved these changes May 14, 2024
View reviewed changes
Copy link
Member

@marclave marclave left a comment

Choose a reason for hiding this comment

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

wow, awesome work @HelgeSverre and @hanspagel for getting this over the finish line!

tested and works GREAT!!!

@marclave marclave merged commit 2a35f2a into scalar:main May 14, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hanspagel hanspagel hanspagel approved these changes

@amritk amritk amritk approved these changes

@marclave marclave marclave approved these changes

Assignees

@hanspagel hanspagel

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@HelgeSverre @hanspagel @amritk @marclave