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: Can no longer access header values sent through JSON helper via loaderHeaders.get #1705

Closed
wants to merge 16 commits into from
Closed

fix: Can no longer access header values sent through JSON helper via loaderHeaders.get #1705

wants to merge 16 commits into from

Conversation

himorishige
Copy link
Contributor

I modified the header values sent by the JSON helper to be accessible via loaderHeaders.get

close #1140

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Jan 29, 2022

Hi @himorishige,

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

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Jan 29, 2022

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

@himorishige himorishige changed the title fix Can no longer access header values sent through JSON helper via loaderHeaders.get fix: Can no longer access header values sent through JSON helper via loaderHeaders.get Jan 29, 2022
Runner-dev and others added 4 commits January 29, 2022 20:50
*Total -- 1,042.02kb -> 923.03kb (11.42%)

/examples/turborepo-vercel/vercel-project-config-example.png -- 163.96kb -> 90.54kb (44.78%)
/examples/image-resize/assets/dog-1.jpg -- 672.67kb -> 628.55kb (6.56%)
/examples/image-resize/assets/other-dogs/dog-2.jpg -- 37.92kb -> 37.13kb (2.08%)
/fixtures/gists-app/app/components/guitar.jpg -- 167.48kb -> 166.81kb (0.4%)

Signed-off-by: ImgBotApp <ImgBotHelp@gmail.com>

Co-authored-by: ImgBotApp <ImgBotHelp@gmail.com>
@ryanflorence
Copy link
Member

You mind writing a test for this? I'm unclear on how the bug surfaces, and why this fixes it.

I'd expect the fix to be in the json helper or the code that calls headers.

@ryanflorence
Copy link
Member

Please see the comments on #1140

@himorishige
Copy link
Contributor Author

himorishige commented Jan 30, 2022

Thanks for reviewing it.
I reverted and fixed the code.

2241639

I'm hoping that in my environment, the header will be no-store.
However, it seemed that max-age... was displayed.

remix 1.1.3

import { HeadersFunction, json, LoaderFunction } from 'remix';

export const headers: HeadersFunction = ({ loaderHeaders }) => {
  console.log(loaderHeaders.get('Cache-Control'));  // return null
  const cacheControl =
    loaderHeaders.get('Cache-Control') ??
    'max-age=0, s-maxage=60, stale-while-revalidate=60';
  return {
    'Cache-Control': cacheControl,
  };
};

export const loader: LoaderFunction = async () => {
  return json(null, { headers: { 'Cache-Control': 'no-store' } });
};

export default function PostsId() {
  return <div className="prose p-4">text</div>;
}

https://codesandbox.io/s/intelligent-sammet-losgp?file=/app/routes/index.tsx

@ryanflorence
Copy link
Member

Ah we have regression here, at the moment, headers incorrectly requires that every parent route has a loader. We'll fix this soon, thanks for your work on this :)

@himorishige
Copy link
Contributor Author

Thank you.
I appreciate that you can fix it.
It seems that this issue is also related to #1518.
This PR will be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants