Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: order editing data model #2184
feat: order editing data model #2184
Changes from 22 commits
97be92e
9625651
c8581f9
d584ea7
08ada35
0343b44
f6fc43b
67fb94f
e899248
224f0fb
cee6c2b
9a02c66
9f2ce1b
1e4b170
c3c86c2
d15e788
5337f5e
041a2aa
efebe13
1f1f23e
673ec00
2ec443b
4d938c6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
comment: same comment as the other for all optional fields that are nullable but can't be undefined
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.
I just followed the established pattern for that case we have in the codebase: for example https://github.com/medusajs/medusa/blob/master/packages/medusa/src/models/batch-job.ts#L49:L65.
If we add
| null
typeorm will in some cases generate a wrong type if explicittype
is not set in the decoratorThere 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.
I didn’t mean to not set it in the decorator. But the problem is that If we manipulate the property you can’t set null even if the field is nullable. Because of the type error you get. Let it that way, but each time we create a new entity i mentioned that point ^^. Also, it is not a pattern since the cart, address, shipping etc specify that null possibility if you look at it. I think it is more an inconsistency and I know that we choose to not change the old entity since it can get you in more refactoring than expected 😂
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.
todo: We should add that in the description below with
[EXPERIMENTAL] first
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.
If you can update the property description in the oas comment bellow with [EXPERIMENTAL]