Tasks / Use Case considerations for refactoring #174

Closed
PWKad opened this Issue Oct 28, 2015 · 17 comments

Comments

Projects
None yet
6 participants
@PWKad
Contributor

PWKad commented Oct 28, 2015

Use cases to cover

  • Validate against POJO objects when used in browser
  • Validate against POJO objects when used on server
  • Show errors in the DOM if the user wants to render them
  • Validate objects and object properties
  • Validate breeze entities in the same manner
  • Custom attribute to traverse the DOM downward and validate forms / divs / tables / etc...

Refactor Tasks for aurelia-validation

  • Remove unneeded dependencies on the DOM
  • Identify where promise dependencies can be reduced
  • Improve the binding and watching properties
  • Remove PathObserver dependency (up for debate)
  • Move debounce to use new debounce from binding (up for debate)
  • Implement hook for triggering ErrorRenderer (start and complete?)
  • Reduce unused properties on results / groups objects
  • ?

Refactor Tasks for aurelia-templating-validation

  • Move all templating-related issues over from validation to templating-validation repo
  • Remove unneeded dependencies leftover from restructuring
  • Move all i18n functionality out
  • Add dependency on i18n to use error message translations
  • Add renderer sample for bootstrap
  • Add renderer sample for another CSS framework
  • Move docs over, fix docs after refactoring
  • Add better customization examples
  • ?
@alvarezmario

This comment has been minimized.

Show comment
Hide comment
@alvarezmario

alvarezmario Oct 28, 2015

Continuation of #161

Continuation of #161

@alvarezmario

This comment has been minimized.

Show comment
Hide comment
@alvarezmario

alvarezmario Oct 28, 2015

I'm not completely sure, but I think i18n functionality should be inside aurelia-validation as is the one in charge of generating the validation error messages. aurelia-templating-validation should only implement the validate custom attribute and the different renderers we support.
Can't we implement a plugable system for that, I mean, have an aurelia-templating-validation-bootstrap or aurelia-templating-validation-foundation? This way we can have help from the community on implementing renderers for any CCS framework.

I'm not completely sure, but I think i18n functionality should be inside aurelia-validation as is the one in charge of generating the validation error messages. aurelia-templating-validation should only implement the validate custom attribute and the different renderers we support.
Can't we implement a plugable system for that, I mean, have an aurelia-templating-validation-bootstrap or aurelia-templating-validation-foundation? This way we can have help from the community on implementing renderers for any CCS framework.

@gishmel

This comment has been minimized.

Show comment
Hide comment
@gishmel

gishmel Oct 28, 2015

I would think that i18n has no place in either library and isn't that what the above task item is pointing out? I think the goal is to make all the i18n use the aurelia-i18n library for that functionality. I agree that these libraries should be pluggable but its important to keep the scope small in order to more easily test and expand when necessary. I will be happy to take some of these tasks for this weekends work.

gishmel commented Oct 28, 2015

I would think that i18n has no place in either library and isn't that what the above task item is pointing out? I think the goal is to make all the i18n use the aurelia-i18n library for that functionality. I agree that these libraries should be pluggable but its important to keep the scope small in order to more easily test and expand when necessary. I will be happy to take some of these tasks for this weekends work.

@jods4

This comment has been minimized.

Show comment
Hide comment
@jods4

jods4 Oct 28, 2015

Contributor

I was about to point out the same.

  • i18n should be built on top of aurelia-i18n capabilities (which I assume is Move all i18n functionality out).
  • but Add dependency on i18n to use error message translations belongs to aurelia-validation. A validation library, even UI-free, is useless if it cannot produce error messages that describe the validation errors.
    Consider potential use-cases: validate command-line arguments on a js tool; or on a server-side (nodejs) web service, which should return a validation failure message to the client. Those don't require aurelia-validation-template (no DOM) but clearly are in need of error messages (preferably localized).
Contributor

jods4 commented Oct 28, 2015

I was about to point out the same.

  • i18n should be built on top of aurelia-i18n capabilities (which I assume is Move all i18n functionality out).
  • but Add dependency on i18n to use error message translations belongs to aurelia-validation. A validation library, even UI-free, is useless if it cannot produce error messages that describe the validation errors.
    Consider potential use-cases: validate command-line arguments on a js tool; or on a server-side (nodejs) web service, which should return a validation failure message to the client. Those don't require aurelia-validation-template (no DOM) but clearly are in need of error messages (preferably localized).
@alvarezmario

This comment has been minimized.

Show comment
Hide comment
@alvarezmario

alvarezmario Oct 28, 2015

Sorry for not being clear enough, but of course I agree with using aurelia-i18n instead of the current aurelia-validation i18n implementation. I've already stated that in other issues. (#161 (comment)).
But the dependency on the aurelia-i18n should be in the main library.

Sorry for not being clear enough, but of course I agree with using aurelia-i18n instead of the current aurelia-validation i18n implementation. I've already stated that in other issues. (#161 (comment)).
But the dependency on the aurelia-i18n should be in the main library.

@gishmel

This comment has been minimized.

Show comment
Hide comment
@gishmel

gishmel Oct 29, 2015

It is probably my fault I misunderstood but yes I absolutely agree that aurelia-i18n should be able to be plugged into aurelia-validation. I just don't understand why one must declare it as a hard dependency. Is it not enough to allow one if so inclined to plug the 2 together if need be but I might argue that it is too much to force using aurelia-i18n when some might not need or want it. Thats just my personal opinion.

gishmel commented Oct 29, 2015

It is probably my fault I misunderstood but yes I absolutely agree that aurelia-i18n should be able to be plugged into aurelia-validation. I just don't understand why one must declare it as a hard dependency. Is it not enough to allow one if so inclined to plug the 2 together if need be but I might argue that it is too much to force using aurelia-i18n when some might not need or want it. Thats just my personal opinion.

@alvarezmario

This comment has been minimized.

Show comment
Hide comment
@alvarezmario

alvarezmario Oct 29, 2015

@gishmel good point. Probably if it could be implemented as being optional, could be better.

@gishmel good point. Probably if it could be implemented as being optional, could be better.

@PWKad

This comment has been minimized.

Show comment
Hide comment
@PWKad

PWKad Nov 3, 2015

Contributor

Aside from the i18n work being fleshed out fully I think the task list above is close to what we need. I'm going to throw some assumptions out below and see how we can divvy up the tasks above if you all are interested. If along the way you see some improvements we can make to reduce complexity let's do it, although I would say let's try not to change the existing functionality outside the scope of each task so we don't end up with a hodge-podge of changes that don't work together ;) -

@gishmel for the ErrorRenderer class I think in a few discussions we've mentioned the need for a class that by default does nothing in the case of a situation like being used server-side or something but provides the ability for templating-validation to hook in to errors being caught in a validation group to render them in a functional manner. To resolve this task templating-validation should also hook in to the validation plugin through this interface using the current functionality to render the errors somehow. Even if this is just a PoC at this point it would be good to see so we all can review how the interoperability works and get some feedback. This would be the first major step towards the 'bigger' refactor goals.

@nomack84 You seem you have a really good handle for how validation and i18n can work together. The resources directory we currently have in validation are great but like you mentioned it's not really a concern of validation for how the messages are shown in the DOM. Would you like to take the task of looking in to how the two libraries can work together to separate out the concerns? It doesn't have to be a fully working PR submitted but at least an outline of how to refactor validation to take advantage of i18n and reduce the necessary number of promises?

@atsu85 If you are willing to continue the work you've been doing thus far on the TWBootstrapViewStrategyBase and making it more extensible I think we'd love for you to work with @gishmel to look at how we can take the work you've done so far with error messaging and tie it in to the templating library so that it's more in line with how Aurelia treats and interacts with the DOM and also so that it can be easier for the consumer (developer) to customize to their needs.

@jods4 Once the dust settles we need to get the computedFrom PRs and tests you have submitted merged (I'm going to try to get this done ASAP) but we need to look at how the validation library is currently observing changes and disposing of subscriptions. Would you want to help take a look at some of the ways that we can make improvements in the validation library around some of the change observation methods?

My focus is going to be on finishing up the splitting of the repositories first (close to complete) and then focusing on making sure the tasks that don't get picked up get completed. If you are all are willing to help with these tasks we can use the gitter channel https://gitter.im/aurelia/validation to discuss changes more in-depth. For each task you would like to take on please let me know and I will mark your name next to it and help to mark the tasks complete.

Contributor

PWKad commented Nov 3, 2015

Aside from the i18n work being fleshed out fully I think the task list above is close to what we need. I'm going to throw some assumptions out below and see how we can divvy up the tasks above if you all are interested. If along the way you see some improvements we can make to reduce complexity let's do it, although I would say let's try not to change the existing functionality outside the scope of each task so we don't end up with a hodge-podge of changes that don't work together ;) -

@gishmel for the ErrorRenderer class I think in a few discussions we've mentioned the need for a class that by default does nothing in the case of a situation like being used server-side or something but provides the ability for templating-validation to hook in to errors being caught in a validation group to render them in a functional manner. To resolve this task templating-validation should also hook in to the validation plugin through this interface using the current functionality to render the errors somehow. Even if this is just a PoC at this point it would be good to see so we all can review how the interoperability works and get some feedback. This would be the first major step towards the 'bigger' refactor goals.

@nomack84 You seem you have a really good handle for how validation and i18n can work together. The resources directory we currently have in validation are great but like you mentioned it's not really a concern of validation for how the messages are shown in the DOM. Would you like to take the task of looking in to how the two libraries can work together to separate out the concerns? It doesn't have to be a fully working PR submitted but at least an outline of how to refactor validation to take advantage of i18n and reduce the necessary number of promises?

@atsu85 If you are willing to continue the work you've been doing thus far on the TWBootstrapViewStrategyBase and making it more extensible I think we'd love for you to work with @gishmel to look at how we can take the work you've done so far with error messaging and tie it in to the templating library so that it's more in line with how Aurelia treats and interacts with the DOM and also so that it can be easier for the consumer (developer) to customize to their needs.

@jods4 Once the dust settles we need to get the computedFrom PRs and tests you have submitted merged (I'm going to try to get this done ASAP) but we need to look at how the validation library is currently observing changes and disposing of subscriptions. Would you want to help take a look at some of the ways that we can make improvements in the validation library around some of the change observation methods?

My focus is going to be on finishing up the splitting of the repositories first (close to complete) and then focusing on making sure the tasks that don't get picked up get completed. If you are all are willing to help with these tasks we can use the gitter channel https://gitter.im/aurelia/validation to discuss changes more in-depth. For each task you would like to take on please let me know and I will mark your name next to it and help to mark the tasks complete.

@alvarezmario

This comment has been minimized.

Show comment
Hide comment
@alvarezmario

alvarezmario Nov 3, 2015

@PWKad I'd like to help with i18n, but currently with all this movements I'm a bit lost on how can I set up the "working stage" to be able to make PR. In response to your answer, I do believe the resources should be in the main validation library, so you can have that as much as possible independent from aurelia. So aurelia-validation should be the one that depends on aurelia-i18n and the one that contains/handle the resources files.
This resources files should/must be simple flat JSON files, just like aurelia-i18n consumed files. @janvanderhaegen once explained to me that there is a reason to have the resources the way they are now, but having aurelia-i18n in place, I think that's not necessary anymore.
So, if you provide me some tips on how to start working now, I'm glad to help and try to do something about i18n for `aurelia-validation library.

@PWKad I'd like to help with i18n, but currently with all this movements I'm a bit lost on how can I set up the "working stage" to be able to make PR. In response to your answer, I do believe the resources should be in the main validation library, so you can have that as much as possible independent from aurelia. So aurelia-validation should be the one that depends on aurelia-i18n and the one that contains/handle the resources files.
This resources files should/must be simple flat JSON files, just like aurelia-i18n consumed files. @janvanderhaegen once explained to me that there is a reason to have the resources the way they are now, but having aurelia-i18n in place, I think that's not necessary anymore.
So, if you provide me some tips on how to start working now, I'm glad to help and try to do something about i18n for `aurelia-validation library.

@atsu85

This comment has been minimized.

Show comment
Hide comment
@atsu85

atsu85 Nov 3, 2015

Contributor

@nomack84

So aurelia-validation should be the one that depends on aurelia-i18n and the one that contains/handle the resources files.

I agree, adding dependency to aurelia-i18n would be better than "reinventing the i18n wheel" in validation lib. Having an abstraction layer for i18n between two libs (validation and aurelia-i18n) could be a bonus (that follows modular and pluggable Aurelia design principle), to allow replacing aurelia-i18n with some other implementation, but i don't think this should be taken as a goal before beta release.

This resources files should/must be simple flat JSON files, just like aurelia-i18n consumed files.

Actually i prefer aurelia-validation approach for storing translations in js files over json format used in aurelia-i18n (I'm not saying that i like translation functions used in aurelia-validation, probably i'd prefer simple exported translation object without any functions that would be equivalent to JSON syntax). Unlike json file format js files can contain:

  1. comments for translation keys
  2. multiline strings and other JS and ES6 language features

But i guess a lot of devs are storing translations for backend and frontend in the same place (for example in DB) and for them it would be easier to produce/serve JSON than js files for front-end. It wouldn't hurt if aurelia-i18n could handle both formats so that those who need translations only for front-end, could use js format (that i'd prefer in that scenario).

Contributor

atsu85 commented Nov 3, 2015

@nomack84

So aurelia-validation should be the one that depends on aurelia-i18n and the one that contains/handle the resources files.

I agree, adding dependency to aurelia-i18n would be better than "reinventing the i18n wheel" in validation lib. Having an abstraction layer for i18n between two libs (validation and aurelia-i18n) could be a bonus (that follows modular and pluggable Aurelia design principle), to allow replacing aurelia-i18n with some other implementation, but i don't think this should be taken as a goal before beta release.

This resources files should/must be simple flat JSON files, just like aurelia-i18n consumed files.

Actually i prefer aurelia-validation approach for storing translations in js files over json format used in aurelia-i18n (I'm not saying that i like translation functions used in aurelia-validation, probably i'd prefer simple exported translation object without any functions that would be equivalent to JSON syntax). Unlike json file format js files can contain:

  1. comments for translation keys
  2. multiline strings and other JS and ES6 language features

But i guess a lot of devs are storing translations for backend and frontend in the same place (for example in DB) and for them it would be easier to produce/serve JSON than js files for front-end. It wouldn't hurt if aurelia-i18n could handle both formats so that those who need translations only for front-end, could use js format (that i'd prefer in that scenario).

@atsu85

This comment has been minimized.

Show comment
Hide comment
@atsu85

atsu85 Nov 3, 2015

Contributor

@PWKad

@atsu85 If you are willing to continue the work you've been doing thus far on the TWBootstrapViewStrategyBase and making it more extensible I think we'd love for you to work with @gishmel to look at how we can take the work you've done so far with error messaging and tie it in to the templating library so that it's more in line with how Aurelia treats and interacts with the DOM and also so that it can be easier for the consumer (developer) to customize to their needs.

After my pull requests got merged, TWBootstrapViewStrategyBase is extensible enough for me (i'm not actually using Twitter Bootstrap, so maybe that class could be renamed now after my refactoring is merged). I'll add the link to my solution here - #168 (comment)

so that it's more in line with how Aurelia treats and interacts with the DOM

I'd love to hear some concrete suggestions, but at the moment i'm not sure how to improve it so it wouldn't become too restrictive. One idea i had in mind can't be done because of issue with content selectors described here:
aurelia/templating#173 (comment)

Contributor

atsu85 commented Nov 3, 2015

@PWKad

@atsu85 If you are willing to continue the work you've been doing thus far on the TWBootstrapViewStrategyBase and making it more extensible I think we'd love for you to work with @gishmel to look at how we can take the work you've done so far with error messaging and tie it in to the templating library so that it's more in line with how Aurelia treats and interacts with the DOM and also so that it can be easier for the consumer (developer) to customize to their needs.

After my pull requests got merged, TWBootstrapViewStrategyBase is extensible enough for me (i'm not actually using Twitter Bootstrap, so maybe that class could be renamed now after my refactoring is merged). I'll add the link to my solution here - #168 (comment)

so that it's more in line with how Aurelia treats and interacts with the DOM

I'd love to hear some concrete suggestions, but at the moment i'm not sure how to improve it so it wouldn't become too restrictive. One idea i had in mind can't be done because of issue with content selectors described here:
aurelia/templating#173 (comment)

@jods4

This comment has been minimized.

Show comment
Hide comment
@jods4

jods4 Nov 3, 2015

Contributor

@PWKad I certainly have ideas around changes observation, but right now I'm overwhelmed by work. I really have zero time for the 1-2 weeks to come, so don't even expect feedback during that period. Sorry :(

Contributor

jods4 commented Nov 3, 2015

@PWKad I certainly have ideas around changes observation, but right now I'm overwhelmed by work. I really have zero time for the 1-2 weeks to come, so don't even expect feedback during that period. Sorry :(

@jods4

This comment has been minimized.

Show comment
Hide comment
@jods4

jods4 Nov 3, 2015

Contributor

PS: the PR that fixes computedFrom is rather trivial and would be very helpful to current aurelia-validation users I think. At least I can tell my project could not run without a locally patched library. I think it may be worth it to merge so that people have something more stable until the refactoring is complete, which might be while I guess...

Contributor

jods4 commented Nov 3, 2015

PS: the PR that fixes computedFrom is rather trivial and would be very helpful to current aurelia-validation users I think. At least I can tell my project could not run without a locally patched library. I think it may be worth it to merge so that people have something more stable until the refactoring is complete, which might be while I guess...

@PWKad

This comment has been minimized.

Show comment
Hide comment
@PWKad

PWKad Jan 19, 2016

Contributor

I would like some feedback on a proposed change for templating -

<template>
  <h1>Validation</h1>
  <div>
    <validation-summary v.bind="validation"></validation-summary>
  </div>
  <label>
    <strong>First Name</strong>
    <input value.bind="firstName" />
    <validation-summary value.bind="firstName" v.bind="validation"></validation-summary>
  </label>
  <label>
    <strong>Last Name</strong>
    <input value.bind="lastName"/>
    <validation-summary value.bind="lastName" v.bind="validation"></validation-summary>
  </label>
  <button click.trigger="validate()">Validate</button>
</template>

Basically this allows the user to specify their own validation-summary. By default it renders the errors for validation and if a value is passed it isolates to only those errors. This allows rendering validation group errors as well as individual items. The v.bind="validation" allows binding to a specific validation group.

By default the current TWBootstrapViewStrategy would be the basis that would be used to show the errors the way bootstrap does but I think this would also make it easier to extend to other libraries without having to understand the internals of Validation.

Any feedback welcome, I'm going to put together two PR's and I'll ping some of you for review there.

Contributor

PWKad commented Jan 19, 2016

I would like some feedback on a proposed change for templating -

<template>
  <h1>Validation</h1>
  <div>
    <validation-summary v.bind="validation"></validation-summary>
  </div>
  <label>
    <strong>First Name</strong>
    <input value.bind="firstName" />
    <validation-summary value.bind="firstName" v.bind="validation"></validation-summary>
  </label>
  <label>
    <strong>Last Name</strong>
    <input value.bind="lastName"/>
    <validation-summary value.bind="lastName" v.bind="validation"></validation-summary>
  </label>
  <button click.trigger="validate()">Validate</button>
</template>

Basically this allows the user to specify their own validation-summary. By default it renders the errors for validation and if a value is passed it isolates to only those errors. This allows rendering validation group errors as well as individual items. The v.bind="validation" allows binding to a specific validation group.

By default the current TWBootstrapViewStrategy would be the basis that would be used to show the errors the way bootstrap does but I think this would also make it easier to extend to other libraries without having to understand the internals of Validation.

Any feedback welcome, I'm going to put together two PR's and I'll ping some of you for review there.

@EisenbergEffect

This comment has been minimized.

Show comment
Hide comment
@EisenbergEffect

EisenbergEffect Jan 19, 2016

Member

@PWKad Have we updated the internals of validation to have a less hacky approach to connecting UI with validation info? That's probably my biggest concern. If you haven't yet, please try to get with @jdanyow to see how he handled the breeze integration. We want to take the same approach. It may not be related directly to this issue, but I want to mention it.

Member

EisenbergEffect commented Jan 19, 2016

@PWKad Have we updated the internals of validation to have a less hacky approach to connecting UI with validation info? That's probably my biggest concern. If you haven't yet, please try to get with @jdanyow to see how he handled the breeze integration. We want to take the same approach. It may not be related directly to this issue, but I want to mention it.

@alvarezmario

This comment has been minimized.

Show comment
Hide comment
@alvarezmario

alvarezmario Jan 19, 2016

How awesome to see activity on the aurelia-validation plugin again. At the moment I think is the weakest point of the library, and this is quite an important plugin.

How awesome to see activity on the aurelia-validation plugin again. At the moment I think is the weakest point of the library, and this is quite an important plugin.

@EisenbergEffect

This comment has been minimized.

Show comment
Hide comment
@EisenbergEffect

EisenbergEffect May 16, 2016

Member

@PWKad Can this issue be closed now?

Member

EisenbergEffect commented May 16, 2016

@PWKad Can this issue be closed now?

@PWKad PWKad closed this May 17, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment