Skip to content

Conversation

@jviide
Copy link
Collaborator

@jviide jviide commented Dec 17, 2018

PR #38 introduces a new custom parser implementing the complete JSX syntax. This pull request adds tests to some corner cases, and suggests fixes for them:

  • JSX allows property names that contain hyphens. This can be fixed by JSON.stringifying the property names.
  • Boolean attributes following non-boolean ones got ignored (e.g. <a b="1" c /> yielded the result h('a', { b: '1' }). This can be fixed by switching to MODE_WHITESPACE when we encounter a whitespace (and after calling commit()).
  • out.replace(/,\(\{(.*?)$/, ',Object.assign({},{$1') gave unexpected results when there were multiple elements with spreads (because e.g. "aba".replace(/a.*?$/, "x") returns "x") as well as attribute values containing the (sub)string ,({. This can be fixed by separately keeping track of the generated property argument.
  • JSX seems to allow NUL (\u0000) in attribute values and text. As the statics were joined with NULs the parser couldn't tell apart original NULs and template variable placeholders. The joined string was also used as a cache key, with similar ambiguity problems as described in PR Use unique keys for caching results #22. This can be fixed by ensuring unique cache keys (similar to Use unique keys for caching results #22) and not joining the statics with NULs.

@jviide
Copy link
Collaborator Author

jviide commented Dec 17, 2018

With the proposed changes the gzipped size stays just under 700 bytes (692 bytes).

@developit
Copy link
Owner

developit commented Dec 17, 2018

awsome, thanks for acting so quick on this! I am going to try to golf a bit post-merge but right now this is only like +2b 💃 (update: ~80b)

@developit developit merged commit 6cb4a7a into developit:custom-jsx-parser Dec 17, 2018
@developit
Copy link
Owner

@jviide hmm - I think I ran into a bug with the revised attribute logic. This test is failing:

html`<div ...${props} ...${other} />`

@jviide jviide deleted the custom-jsx-parser branch December 17, 2018 22:11
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.

2 participants