Skip to content

Quote only names that need to be quoted, not the reverse#5641

Merged
RyanCavanaugh merged 2 commits into
microsoft:masterfrom
RyanCavanaugh:fix5634
Nov 12, 2015
Merged

Quote only names that need to be quoted, not the reverse#5641
RyanCavanaugh merged 2 commits into
microsoft:masterfrom
RyanCavanaugh:fix5634

Conversation

@RyanCavanaugh

Copy link
Copy Markdown
Member

Fixes #5634

The code for detecting when to quote the property names in object literals when emitting React was spectacularly wrong. On account of being doubly wrong, this turned out to be mostly right, but it was still really bad:

  • The logic was backwards -- it quoted things that didn't need quoting
  • It didn't check the entire string, so it thought x-y didn't need quoting because it matched the substring x.
  • The * character was inside the [], which meant we were looking for a literal * instead of "0 or more" of the thing inside the []

@DanielRosenwasser

Copy link
Copy Markdown
Member

👍

1 similar comment
@vladima

vladima commented Nov 12, 2015

Copy link
Copy Markdown
Contributor

👍

Comment thread src/compiler/emitter.ts Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  1. the brackets are not needed (as we discussed in person).
  2. Why have + after [A-Za-z_]? I would think that \w* is enough on its own to match identifier characters.
  3. Does \w match the characters that are valid in Unicode identifiers? IS there any easy way to match those characters?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Re 3, I don't know. The failure mode here is that we quote something that didn't need to be quoted, which isn't really a problem per se

RyanCavanaugh added a commit that referenced this pull request Nov 12, 2015
Quote only names that need to be quoted, not the reverse
@RyanCavanaugh RyanCavanaugh merged commit e3a845a into microsoft:master Nov 12, 2015
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants