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(remix-server-runtime): inherit parent route's headers if child route doesn't export headers #5473

Merged
merged 1 commit into from
May 19, 2023
Merged

feat(remix-server-runtime): inherit parent route's headers if child route doesn't export headers #5473

merged 1 commit into from
May 19, 2023

Conversation

appden
Copy link
Contributor

@appden appden commented Feb 15, 2023

I've found it very surprising and have been bitten a few times that a parent route's headers are not automatically used by a child route when that child route does not export its own headers. This proposes doing just that for v2 since this would be a breaking change.

@changeset-bot
Copy link

changeset-bot bot commented Feb 15, 2023 •
edited
Loading

⚠️ No Changeset found

Latest commit: 6763cad

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

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Feb 15, 2023

Hi @appden,

Welcome, and thank you for contributing to Remix!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run.

Thanks!

- The Remix team

@github-actions
Copy link
Contributor

This PR has been automatically marked stale because we haven't received a response from the original author in a while 🙈. This automation helps keep the issue tracker clean from issues that are not actionable. Please reach out if you have more information for us or you think this issue shouldn't be closed! 🙂 If you don't do so within 7 days, this PR will be automatically closed.

@github-actions github-actions bot added the needs-response We need a response from the original author about this issue/PR label Apr 17, 2023
@MichaelDeBoey MichaelDeBoey changed the title Proposal: v2_headers flag to inherit parentHeaders feat(remix-server-runtime): inherit parent route's headers if child route doesn't export headers Apr 17, 2023
@github-actions github-actions bot removed the needs-response We need a response from the original author about this issue/PR label Apr 17, 2023
@MichaelDeBoey MichaelDeBoey added needs-response We need a response from the original author about this issue/PR and removed needs-response We need a response from the original author about this issue/PR labels May 2, 2023
@MichaelDeBoey MichaelDeBoey changed the base branch from main to dev May 2, 2023 01:12
@MichaelDeBoey MichaelDeBoey changed the base branch from dev to main May 2, 2023 01:13
@MichaelDeBoey
Copy link
Member

Hi @appden!

Are you still interested in getting this one merged?

If so, please rebase onto latest dev & fix failing tests

@MichaelDeBoey MichaelDeBoey added the needs-response We need a response from the original author about this issue/PR label May 2, 2023
@appden appden changed the base branch from main to dev May 2, 2023 22:32
@appden
Copy link
Contributor Author

appden commented May 2, 2023

Thanks @MichaelDeBoey, I just rebased and fixed that type error for the tests. If this proposal sounds good (I recall @ryanflorence also wanted to take a look), I can add a test for this new functionality and also update to default this new flag to true on the templates.

@github-actions github-actions bot removed the needs-response We need a response from the original author about this issue/PR label May 2, 2023
@appden
Copy link
Contributor Author

appden commented May 16, 2023

@MichaelDeBoey @ryanflorence kindly following up on this, thanks!

@appden @MichaelDeBoey
Proposal: v2_headers flag to inherit parentHeaders
6763cad 
I've found it very surprising and have been bitten a few times that a parent route's headers are not automatically used by a child route when that child route does not export its own `headers`. This proposes doing just that for v2 since this would be a breaking change.
@brophdawg11 brophdawg11 mentioned this pull request May 19, 2023
Closed
@brophdawg11 brophdawg11 changed the base branch from dev to brophdawg11/v2-headers May 19, 2023 18:26
@brophdawg11
Copy link
Contributor

Thanks @appden! It turns out this was never the intentional behavior, and prior to the v1 release it behaved the way you would expect (the way this Pr is changing it to behave). We agree it's best to put this behind a v2 flag since it released in v1 with the current behavior.

This is going to change/conflict slightly with some work I just merged for errorHeaders in #6425 so I'm going to merge this into a branch of mine and get it synced up and then merge it to dev. I'll keep your name on the commits throughout as a co-author!

@brophdawg11 brophdawg11 merged commit 8dab8e8 into remix-run:brophdawg11/v2-headers May 19, 2023
@appden
Copy link
Contributor Author

appden commented May 19, 2023

Awesome, thanks @brophdawg11!

@appden appden deleted the parent-headers-proposal branch May 19, 2023 18:35
@brophdawg11 brophdawg11 mentioned this pull request May 19, 2023
Merged
@brophdawg11
Copy link
Contributor

Continued in #6431

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

@brophdawg11 brophdawg11 Awaiting requested review from brophdawg11

@mcansh mcansh Awaiting requested review from mcansh

@jacob-ebey jacob-ebey Awaiting requested review from jacob-ebey

Assignees
No one assigned
Labels
CLA Signed package:dev package:server-runtime renderer:react
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@appden @MichaelDeBoey @brophdawg11