-
Notifications
You must be signed in to change notification settings - Fork 10
Remove mappings in validationEngine #40
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
Conversation
Removed @types/whatwg-fetch which caused "Duplicate identifier" errors (see http://stackoverflow.com/a/42429014).
| import { consts } from '../consts'; | ||
|
|
||
| //TODO: Implement Issue #20 | ||
| describe('ValidationEngine Validate Form', () => { |
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 think mocha, chai and core-js imports are not necessary, isn't 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.
It would be better to do it in another issue.
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.
Nice
|
|
||
| it('Spec #2 => should return a succeeded FormValidationResult with one fieldErrors equals { succeeded: true, key: "nameId", type: "REQUIRED", errorMessage: "" }' + | ||
| 'When passing one validation rule with key equals "nameId" and viewModel equals { id: "1", fullname: "john" }', (done) => { | ||
| it('Spec #2 => should return a succeeded FormValidationResult with one fieldErrors equals ' + |
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 think that we can use backticks for this kind of multiline spec description
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 was thinking about that... but remember that even if we use backticks messages may be displayed with same indentation they have in the text editor (at least we don't use 4-8 tabs wide)...
| formFieldToViewModelKeyValues.forEach((formFieldToViewModel: FormNameToFieldNameMapping) => { | ||
| const vmFieldValue = vm[formFieldToViewModel.vmFieldName]; | ||
| if (this.areParametersDefined(vm, validationFn)) { | ||
| for (let vmKey in vm) { |
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.
Other iteration could be using Object.keys(vm).forEach(...) but it's ok with that
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.
Yep, more beautiful, but more steps... (it was my first approach).
| "@types/react-redux": "^4.4.32", | ||
| "@types/redux": "^3.6.31", | ||
| "@types/redux-thunk": "^2.1.31", | ||
| "@types/whatwg-fetch": "0.0.32", |
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.
Why is this typings not necessary?
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.
New versions of TypeScript already have fetch typings. It was causing duplicating export conflicts, see this
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.
Finally we have to uninstall whatwg-fetch lib?
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.
No we don't. The polyfill is still needed but typings.
|
@JaimeSalas and my self reviewed, pending on @nasdan feedback, once you are ok with him please proceed with the merge. Thanks. |
|
Finally, what version should be used for this update?? |
This closes #37.