-
Notifications
You must be signed in to change notification settings - Fork 1
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(fieldconfig): decorator is added #76
Conversation
} | ||
|
||
ret.push(fieldConfig); | ||
// ret.push({ |
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.
delete comment or add TODO if it is useful for near future update
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.
Done
|
||
const fieldConfigMetadataKey = Symbol('fieldconfigclass'); | ||
type KeyValueDict = Record<string, unknown>; | ||
const target = {}; |
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 should have thought about it earlier but since target is static and global it can be used to store the data directly without using reflect metadata.
it will make the code bit simpler to understand.
up to you if u want to change it, its ok either way by my standard (for this use case)
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.
Done, reflection is removed, more clean code
This pull request introduces 2 alerts when merging cbb9db1 into 2c885c4 - view on LGTM.com new alerts:
|
|
||
const classDataList = getFieldConfigClassesInfo(); | ||
classDataList.push(classData); | ||
target.concat(classDataList); |
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.
concat doesn't update the target in js but returns new array
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.
Done, working directly with target
classDataList.push(classData); | ||
Reflect.defineMetadata(graphQLMetadataKey, classDataList, target); | ||
target.concat(classDataList); |
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.
concat doesn't update the target in js but returns new array
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.
Done, working directly with target
New fieldConfig decorator is added