-
-
Notifications
You must be signed in to change notification settings - Fork 415
feat: add immutable types #1350
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 8f8b488 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@smorimoto I'm a little bit unsure on when to use lodash or not in the lib, seems like there is no clear pattern on when it's applied or not. Do you have time to review the PR? |
This comment was marked as outdated.
This comment was marked as outdated.
@smorimoto I fixed the suggestion from cursor |
This comment was marked as outdated.
This comment was marked as outdated.
@smorimoto Could these changes be approved, merged and released? |
8689c2d
to
25634c6
Compare
This comment was marked as outdated.
This comment was marked as outdated.
@smorimoto Sorry for all the pings, but would be amazing with this feature. Is it ready to be merged? |
@smorimoto Any updates, can we aim to have this merged? |
I will take a look at this today. Sorry for being late. Also, how about simply making the flag “immutable”? I think it's simpler. |
Yes I agree, i tried following naming convention of other flags, hence the "make" part. But "immutable" is for sure better, will update it |
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codex Review: Here are some suggestions.
swagger-typescript-api/types/index.ts
Lines 33 to 35 in 25634c6
ArrayType: (content: unknown) => string; | |
StringValue: (content: unknown) => string; | |
BooleanValue: (content: unknown) => string; |
[P1] Align CodeGenConstruct type signatures with new immutable parameters
The CodeGenConstruct
typings still describe helpers like ArrayType
, RecordType
, InterfaceDynamicField
, and Tuple
as unary functions receiving a single content
value. The implementation in CodeGenConfig.Ts
now calls these helpers with objects that include a readonly
flag (this.config.Ts.ArrayType({ readonly, content })
, etc.). Any consumer overriding these constructs using the published types will still implement the old (content) => …
signature and will now receive an object, resulting in [object Object]
output or other breakage when the override is invoked. The declaration should be updated to reflect the new parameter shape or the runtime should continue supporting the old signature to avoid breaking existing custom code.
Reply with @codex fix comments
to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
@smorimoto I changed from makeImmutable to just immutable |
In a functional codebase consumers of
swagger-typescript-api
would want to force properties and values to be immutable even if swagger spec does not havereadOnly
property.With this change one can simply add
--immutable
flag or setimmutable
option to force immutable types.