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

Quote object literal property names when required #36

Merged
merged 2 commits into from
Feb 7, 2024

Conversation

colonel-sanders
Copy link
Contributor

Quote object literal property names when required

In a JS/TS object literal, there are some restrictions on the definition of a property name. For example, this is not a valid object literal:

const cat = {
   9_lives: true
};

because a property name literal is not allowed to begin with a digit. To work around this, we can use a string literal for the property name:

const cat = {
  "9_lives": true
};

If a schema happens to define an object property that requires wrapping quotes, the generated Typescript code isn't currently syntactically correct.

Discussion link: #35

@colonel-sanders
Copy link
Contributor Author

Forgot to mention: I did consider one alternative implementation here. Another valid approach to this problem would be to always use string literals for property names and then let the prettier run (as controlled by the quote-props setting) adjust it to match the user's preference.

@xddq
Copy link
Owner

xddq commented Feb 6, 2024

Hey @colonel-sanders !

I see, thanks for the PR. The only problem I currently have is that I don't understand the regex at first sight.

I would tend towards the second approach, regex matching sometimes leaves some unwanted/unexpected cases open. However, I am wondering why it is not already applied yet, as "as_needed" seems to be the default setting for prettier. And for the case you mentioned it seems to be "needed". We already do "post-processing" with the imports plugin for prettier.

Will take a closer look probably this week.

@colonel-sanders
Copy link
Contributor Author

However, I am wondering why it is not already applied yet, as "as_needed" seems to be the default setting for prettier. And for the case you mentioned it seems to be "needed". We already do "post-processing" with the imports plugin for prettier.

Thanks for taking a look, @xddq! I'd definitely be on-board for using string constants across the board; the default prettier settings will then rewrite that to the style most developers would expect.

Reading my earlier text, I think I wasn't abundantly clear about the failure mode here. To clarify: if I attempt to process this schema with schema2typebox@1.7.0:

{
  "$id": "https://example.com/interesting-prop-naming.json",
  "title": "InterestingPropNaming",
  "type": "object",
  "properties": {
    "@type": { "type": "string" }
  }
}

the prettier run actually errors out, because the input Typescript isn't valid:

SyntaxError: Property assignment expected. (6:51)
  4 |
  5 | export type InterestingPropNaming = Static<typeof InterestingPropNaming>
  6 | export const InterestingPropNaming = Type.Object({@type: Type.Optional(Type.String())}, {"$id":"https://example.com/interesting-prop-naming.json"})
    |                                                   ^
    at v (/.../node_modules/schema2typebox/node_modules/prettier/parser-typescript.js:1:14679)

…e with quotes in properties

- write changelog entry, bump package.json as 1.7.1 release preparation.
@xddq
Copy link
Owner

xddq commented Feb 6, 2024

@colonel-sanders did small changes to your code, but basically just used your approach and updated the function name. Will probably merge and release 1.7.1 in this or the next week. If you have time feel free to check the changes and approve/disapprove or comment.

I did drop your better handling when json schemas have escaped strings in favor of an easier to understand function. If this will be needed we can add it again, but the json schemas just seem weird like that.

Thanks again for the PR.

@colonel-sanders
Copy link
Contributor Author

If you have time feel free to check the changes and approve/disapprove or comment.

Looks great. Thanks!

@xddq xddq merged commit cba56eb into xddq:main Feb 7, 2024
8 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

2 participants