feat(field): add id field type #52
Conversation
Adding ID field option on metadata. herbsjs#46
Codecov Report
@@ Coverage Diff @@
## master #52 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 3 4 +1
Lines 103 108 +5
=========================================
+ Hits 103 108 +5
Continue to review full report at Codecov.
|
README.md
Outdated
| }) | ||
| ``` | ||
|
|
||
| ID Fields only accepts ```Number``` or ```String``` type. |
There was a problem hiding this comment.
How a id typed as Date is a problem? It might not look right, but I can't see why we should constraint the type. It is a developer decision that won't impact in any way Herbs architecture.
There was a problem hiding this comment.
I remove this in the documentation.
| const { field } = require('../field') | ||
|
|
||
| const id = (type, options) => { | ||
| return field(type, { ...options, isId: true }) |
There was a problem hiding this comment.
const Plan =
entity('Plan', {
...
myId: id(Number, { validation: { length: { is: 10 } }),Is it possible to have the above code with this implementation?
There was a problem hiding this comment.
Yes, should work fine. I've added tests with this scenario
src/field.js
Outdated
| this.type = type | ||
| this.options = options | ||
| this.isId = !!options.isId |
There was a problem hiding this comment.
Should it be replicated?
if options.isID is true the object will have meta.options.isID and meta.isID fields. Is it necessary?
There was a problem hiding this comment.
Yes, should.
Because validation method inside baseEntity.js access the meta.schema not the options.
There was a problem hiding this comment.
is it possible to have:
if (options && options.exceptIDs && definition.options.isId) continue?
src/field.js
Outdated
| this.type = type | ||
| this.options = options | ||
| this.isId = !!options.isId |
There was a problem hiding this comment.
is it possible to have:
if (options && options.exceptIDs && definition.options.isId) continue?
… options inside baseEntity
README.md
Outdated
|
|
||
| ```javascript | ||
|
|
||
| const instance = User() |
There was a problem hiding this comment.
const instance = User() should be const instance = new User()?
is this a new instance or a entity type (class)?
There was a problem hiding this comment.
You're right, I fixed the docs
test/entity/multipleFieldsEntity.js
Outdated
| @@ -131,5 +136,67 @@ describe('An entity', () => { | |||
| }) | |||
| }) | |||
|
|
|||
| it('should set a field as ID using id module', () => { | |||
There was a problem hiding this comment.
'should set a field as ID using id module'
what is the 'id module'?
should it be 'should set a field as ID and have valid entity'?
There was a problem hiding this comment.
I agree with you, it was inside the old PR, but module isn't the best message for this.
|
I think this PR is ready to merge. However, it should be merged only if the documentation on the web site is ready to merge as well. |
|
@dalssoft ok, I'll do it |
|
🎉 This PR is included in version 1.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
fix buchu bug in audittrail (herbsjs/buchu#52). Add ID field feature in gotu (herbsjs/gotu#52). Add checker isIterable in suma (herbsjs/suma#36). Add javascriptIdentifier in suma (herbsjs/suma#38)
Adding ID field option on metadata.
Took the liberty to add a type validation for ID fields (only Number and String), if the wrong type is sent, gotu will ignore the isID option. Hope it's consensus among all that we need something like that, but I did not implemented nothing fancy for now.
finishing pr #48
Fixes #46
Proposed Changes
Readiness Checklist
Author/Contributor
Reviewing Maintainer
breakingif this is a large fundamental changeautomation,bug,documentation,enhancement,infrastructure, orperformance