-
-
Notifications
You must be signed in to change notification settings - Fork 492
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
Change all project enums to objects with as const #2445
Conversation
Thanks for this. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2445 +/- ##
========================================
Coverage 99.94% 99.94%
========================================
Files 329 329
Lines 13188 13311 +123
Branches 1367 1309 -58
========================================
+ Hits 13181 13304 +123
Misses 7 7 ☔ View full report in Codecov by Sentry. |
35218d6
to
517ae1b
Compare
export const DocumentGridType = { | ||
DEFAULT: "default", | ||
LINES: "lines", | ||
LINES_AND_CHARS: "linesAndChars", | ||
SNAP_TO_CHARS: "snapToChars", | ||
} as const; |
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.
having this is beneficial, but it may be challenging to use types in the code base and not having clean types, Additionally using PascalCase for variables is not considered a good practice I would recommend reconsidering the implementation of the types like this:
export const DocumentGridType = { | |
DEFAULT: "default", | |
LINES: "lines", | |
LINES_AND_CHARS: "linesAndChars", | |
SNAP_TO_CHARS: "snapToChars", | |
} as const; | |
const documentGridType = { | |
DEFAULT: "default", | |
LINES: "lines", | |
LINES_AND_CHARS: "linesAndChars", | |
SNAP_TO_CHARS: "snapToChars", | |
} as const; | |
export type DocumentGridType = (typeof documentGridType)[keyof typeof documentGridType] |
or refer to the TypeScript documentation
export const DocumentGridType = { | |
DEFAULT: "default", | |
LINES: "lines", | |
LINES_AND_CHARS: "linesAndChars", | |
SNAP_TO_CHARS: "snapToChars", | |
} as const; | |
const ODocumentGridType = { | |
DEFAULT: "default", | |
LINES: "lines", | |
LINES_AND_CHARS: "linesAndChars", | |
SNAP_TO_CHARS: "snapToChars", | |
} as const; | |
export type DocumentGridType = (typeof documentGridType)[keyof typeof documentGridType] |
I can make a pr to fix it
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've tried to keep naming of enums, otherwise projects after update package version will fall to errors
If talk about pascal, camel, etc. cases, I'd like to use upper case to show their constant state
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 we make a breaking change, then it has to be a v9 update
I'd like to use upper case to show their constant state
Yes UPPER_CASE to show constants sounds nice to me
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 forked remark-docx which uses this project and was on version 8.2.0 and this was a breaking change. Thanks to @hshoja for providing a solution. I am no TS expert and never saw that spending hours trying to figure it out.
Enums after compile looks so gross and they needs a little more space in comparison with objects. This PR should reduce final bundle size
Official documentation say that we dont need to use enums if we can use objects with as const
So that forced me to do this PR