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

feat(jsx/precompile): Normalization and stringification of attribute values as renderToString #3432

Merged
merged 5 commits into from
Sep 24, 2024

Conversation

usualoma
Copy link
Member

fix #3427

The “precompile” process converts attribute values in the same way as the normal renderToString process.
https://github.com/usualoma/hono/blob/fd80df6a0f03876cf6ac2b12b4b7b812c19ca616/src/jsx/base.ts#L180-L227

The author should do the following, if applicable

  • Add tests
  • Run tests
  • bun run format:fix && bun run lint:fix to format the code

Copy link

codecov bot commented Sep 20, 2024

Codecov Report

Attention: Patch coverage is 3.22581% with 30 lines in your changes missing coverage. Please review.

Project coverage is 95.50%. Comparing base (dfbd717) to head (4405e69).
Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
src/jsx/jsx-runtime.ts 0.00% 30 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3432      +/-   ##
==========================================
- Coverage   95.77%   95.50%   -0.28%     
==========================================
  Files         155      155              
  Lines        9310     9337      +27     
  Branches     2725     2865     +140     
==========================================
  Hits         8917     8917              
- Misses        393      420      +27     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@usualoma
Copy link
Member Author

It'll need some tweaking.

@usualoma
Copy link
Member Author

@yusukebe
Would you please review?

src/jsx/jsx-runtime.ts Outdated Show resolved Hide resolved
typeof value === 'string' ? raw(name + '="' + html`${value}` + '"') : html`${name}="${value}"`
key: string,
v: string | Promise<string> | Record<string, string | number | null | undefined | boolean>
): HtmlEscapedString | Promise<HtmlEscapedString> => {
Copy link
Member

Choose a reason for hiding this comment

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

Can we create one common function for these lines and https://github.com/usualoma/hono/blob/fd80df6a0f03876cf6ac2b12b4b7b812c19ca616/src/jsx/base.ts#L180-L227 and use it from the both?

Like this, though, I think objectToAttribute is not the best name.

export const objectToAttribute = (key: string, v: any, buffer: any) => {
  if (v === null || v === undefined) {
    return
  }
  if (key === 'style' && typeof v === 'object') {
    // object to style strings
    let styleStr = ''
    styleObjectForEach(v as Record<string, string | number>, (property, value) => {
      if (value != null) {
        styleStr += `${styleStr ? ';' : ''}${property}:${value}`
      }
    })
    escapeToBuffer(styleStr, buffer)
    buffer[0] += '"'
  } else if (typeof v === 'boolean' && booleanAttributes.includes(key)) {
    return buffer
  } else if (typeof v === 'string') {
  // ...
  return buffer
}

Copy link
Member Author

Choose a reason for hiding this comment

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

@yusukebe
I also considered making it common, but if it is made common, there will inevitably be overhead for both sides in the following areas.

https://github.com/usualoma/hono/blob/4405e69ae8ab419b2dab36dfd25c605a9c50fca3/src/jsx/base.ts#L202
https://github.com/usualoma/hono/blob/4405e69ae8ab419b2dab36dfd25c605a9c50fca3/src/jsx/base.ts#L205

I don't think it's a big overhead, but I want to be particularly conscious of performance when stringifying JSX, so I want to keep it separate for each implementation for optimization. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

@usualoma

Understood well. Let's go with this. Thank you for spending for considering it!

@yusukebe
Copy link
Member

Hey @usualoma

Thank you for the PR. I've left comments!

@usualoma
Copy link
Member Author

Key normalization is done by deno, so processing by hono was removed. 4405e69

Copy link
Member

@yusukebe yusukebe left a comment

Choose a reason for hiding this comment

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

LGTM!

@yusukebe
Copy link
Member

@usualoma

Looks good! Thank you.

@yusukebe yusukebe merged commit 7779fca into honojs:main Sep 24, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

style= objects do not work with jsx: precompile on Deno
2 participants