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: error to retrieve cookie #1386

Merged
merged 2 commits into from
Sep 18, 2023
Merged

fix: error to retrieve cookie #1386

merged 2 commits into from
Sep 18, 2023

Conversation

zeep-chat
Copy link
Contributor

TypeError: Cannot read properties of undefined (reading 'split')

Author should do the followings, if applicable

  • Add tests
  • Run tests
  • yarn denoify to generate files for Deno

TypeError: Cannot read properties of undefined (reading 'split')
return obj[key]
}
if (!cookie) return {}
const obj = parse(cookie)
const obj = parse(cookie, key)
Copy link
Member

Choose a reason for hiding this comment

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

This change is not necessary.

@yusukebe
Copy link
Member

Hi @uaichat

Thank you for the PR.

I've left a comment and there are two things what you want to do:

  1. Add a test.

Please add the following test in src/helper/cookie/index.ts:

    describe('get null if the value is undefined', () => {
      const app = new Hono()

      app.get('/cookie', (c) => {
        const yummyCookie = getCookie(c, 'yummy_cookie')
        const res = new Response('Good cookie')
        if (yummyCookie) res.headers.set('Yummy-Cookie', yummyCookie)
        return res
      })

      it('Should be null', async () => {
        const req = new Request('http://localhost/cookie')
        const cookieString = 'yummy_cookie='
        req.headers.set('Cookie', cookieString)
        const res = await app.request(req)
        expect(res.headers.get('Yummy-Cookie')).toBe(null)
      })
    })
  1. denoify

Please run denoify on the root directory and git add&commit&push it.

@CyberFlameGO
Copy link
Contributor

@yusukebe is the option for maintainer edits enabled on their branch? Perhaps you could add said test and merge it instead of waiting if so as the PR seems stale :)

@yusukebe
Copy link
Member

@CyberFlameGO

Ah, yes. It's enabled. I'll fix and merge this later!

@yusukebe yusukebe merged commit 94786a7 into honojs:main Sep 18, 2023
10 checks passed
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.

None yet

3 participants