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

Fix: Show legacy web headers #1911

Merged
merged 6 commits into from
Mar 8, 2023
Merged

Fix: Show legacy web headers #1911

merged 6 commits into from
Mar 8, 2023

Conversation

finnar-bin
Copy link
Contributor

Closes #497

Preview

Screenshot from 2023-03-07 13-56-10

@finnar-bin finnar-bin requested a review from agalin920 March 7, 2023 05:56
@finnar-bin finnar-bin self-assigned this Mar 7, 2023
import { getResponseData, prepareHeaders } from "./util";
import { LegacyHeader } from "./types";

export const headTagApi = createApi({
Copy link
Contributor

Choose a reason for hiding this comment

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

Our pattern isn't a new rtk query api per endpoint. It's per service. This endpoint is still on the instance service

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, moved to instance.ts

getLegacyHeadTags: builder.query<LegacyHeader[], void>({
query: () => `/web/headers`,
transformResponse: getResponseData,
keepUnusedDataFor: 0.0001,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this need to exclude caching?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot to remove this when I copied the the block off of an already existing query

query: () => `/web/headers`,
transformResponse: getResponseData,
keepUnusedDataFor: 0.0001,
providesTags: () => [{ type: "LegacyHeadTags" }],
Copy link
Contributor

Choose a reason for hiding this comment

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

Providing tags here doesn't seem useful since nothing is invalidating it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed on latest update

@finnar-bin finnar-bin requested a review from agalin920 March 7, 2023 06:26
@shrunyan shrunyan merged commit 32d4a98 into master Mar 8, 2023
@shrunyan shrunyan deleted the fix/497-legacy-web-headers branch March 8, 2023 22:58
@giseleblair
Copy link
Contributor

@theofficialnar besides the warning that there are legacy headtags present, do we display which headtags are legacy?

@finnar-bin
Copy link
Contributor Author

finnar-bin commented Mar 8, 2023

@theofficialnar besides the warning that there are legacy headtags present, do we display which headtags are legacy?

@giseleblair yup, I've added it to the preview section of the head tag. Only issue I realized just now is that, the users will probably need to scan through the input fields first to find out which of these are considered legacy since I haven't indicated it on the preview - I honestly thought we just had to also make sure it's added there.

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.

Legacy web headers do not display in the UI and cannot be deleted via API
4 participants