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

add util for handling remix headers generally #810

Merged
merged 5 commits into from
Jan 17, 2025
Merged

add util for handling remix headers generally #810

merged 5 commits into from
Jan 17, 2025

Conversation

nichtsam
Copy link
Contributor

@nichtsam nichtsam commented Aug 3, 2024 •
edited
Loading

It's a bit tedious to manually merge Remix headers since the process is mostly the same.
This PR adds a utility to streamline that process.

Related Discussion #809
More details on the utility: https://gist.github.com/nichtsam/bff3a0e58f7edfad3985e9322582f67f

Test Plan

  • There are test cases for various functionalities.
  • Test cases with real use cases ensure the utility remains practical.

Checklist

  • Tests updated
  • Docs updated

@nichtsam nichtsam force-pushed the pr/general-headers-util branch from 0d4fdfd to 6c36dca Compare August 3, 2024 14:39
@nichtsam nichtsam marked this pull request as draft August 3, 2024 14:41
@nichtsam
Copy link
Contributor Author

nichtsam commented Aug 3, 2024 •
edited
Loading

Just for the record, reason for switching to draft: privatenumber/tsx#627
This is affecting vite and causing imports behavior to differentiate between tsx and node.

@nichtsam nichtsam force-pushed the pr/general-headers-util branch 2 times, most recently from f9f6ccf to 6fae52e Compare December 4, 2024 17:02
@nichtsam
Copy link
Contributor Author

nichtsam commented Dec 4, 2024

Just for the record, reason for switching to draft: privatenumber/tsx#627 This is affecting vite and causing imports behavior to differentiate between tsx and node.

I switch to another ESM cache-control parser to avoid encountering this issue.

@nichtsam nichtsam marked this pull request as ready for review December 4, 2024 17:06
@nichtsam nichtsam force-pushed the pr/general-headers-util branch 2 times, most recently from f8356fd to 161d544 Compare December 4, 2024 17:13
@kentcdodds
Copy link
Member

I think React Router v7 (single fetch specifically) changes how headers work a fair bit so I don't think I'll be merging this since I want to get on RR7 soon. Sorry I didn't get to you on this sooner!

@nichtsam
Copy link
Contributor Author

nichtsam commented Dec 7, 2024

I think React Router v7 (single fetch specifically) changes how headers work a fair bit so I don't think I'll be merging this since I want to get on RR7 soon. Sorry I didn't get to you on this sooner!

That’s totally fine! I was also thinking that single fetch might impact things.
Just wanted to finish up the PR. No worries, and thanks for the update!

@kentcdodds
Copy link
Member

RR7 has been merged! Feel free to check whether this is necessary and update things if so. Thanks!

@nichtsam nichtsam force-pushed the pr/general-headers-util branch 2 times, most recently from 88eaf17 to bae3048 Compare January 16, 2025 11:42
@nichtsam nichtsam force-pushed the pr/general-headers-util branch from bae3048 to f40e9b0 Compare January 16, 2025 11:45
@nichtsam
Copy link
Contributor Author

RR7 has been merged! Feel free to check whether this is necessary and update things if so. Thanks!

I've rebased onto main.
I’ve tested the behavior of headers, and it appears to remain unchanged, where:

  • Either loaderHeaders or actionHeaders will have a value, but not both at the same time.
  • errorHeaders will be present when an error occurs and will mirror the value of either loaderHeaders or actionHeaders.

kentcdodds
kentcdodds requested changes Jan 16, 2025
View reviewed changes
Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Thanks! Just two changes.

app/utils/remix.server.ts Outdated
@@ -0,0 +1,103 @@
import { type CacheControlValue, parse, format } from '@tusbar/cache-control'
Copy link
Member

Choose a reason for hiding this comment

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

I think calling this file remix.server.ts may not be the right call anymore. Maybe this should just be called headers.server.ts.

Also, could you add some jsdoc comments to these utilities? Thanks!

app/utils/remix.server.test.ts Outdated
import { describe, expect, test } from 'vitest'
import { getConservativeCacheControl } from './remix.server.ts'

describe('getConservativeCacheControl', () => {
Copy link
Member

Choose a reason for hiding this comment

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

I prefer to not use describe because it's unnecessary. Just put the test blocks at the root of the file.

@nichtsam nichtsam requested a review from kentcdodds January 16, 2025 23:06
@nichtsam nichtsam closed this Jan 17, 2025
@nichtsam nichtsam deleted the pr/general-headers-util branch January 17, 2025 00:22
@nichtsam nichtsam restored the pr/general-headers-util branch January 17, 2025 00:23
@nichtsam nichtsam reopened this Jan 17, 2025
@kentcdodds kentcdodds merged commit 5c5328b into epicweb-dev:main Jan 17, 2025
5 checks passed
@nichtsam nichtsam deleted the pr/general-headers-util branch January 17, 2025 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kentcdodds kentcdodds Awaiting requested review from kentcdodds

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nichtsam @kentcdodds