upgrade ember changeset to v3

[jelhan]

Based on #18, which should be merged before. Need to resolve the failing tests before.

[jelhan]

Created an issue upstream to investigate what is going on: adopted-ember-addons/ember-changeset-validations#226

[snewcomer]

Just to verify, can you check the function based approach?

import { Changeset } from 'ember-changeset';

[jelhan]

I'm currently in vacation. Can't check before end of next week. 😟

[jelhan]

Done in d59192d. Same results.

[snewcomer]

Should be an easy change over for ya. The class based approach will be deprecated at some point.

[BryanHunt]

I tried lifting the code from this project and adding it directly to my application. After some hacking, I got this to work - not sure if it is the best solution:

import BsFormElement from 'ember-bootstrap/components/bs-form/element';
import { tracked } from '@glimmer/tracking';

export default class BsFormElementComponent extends BsFormElement {
  @tracked errors;
  hasValidator = true;
  setupValidations() {
    const self = this;
    this.model.on('afterValidation', (key) => {
      if(key == self.property) {
        const errors = self.model.error[key];
        self.errors = errors ? errors.validation : null;
      }
    });
  }
}

[jelhan]

@BryanHunt If I get you right, this code is causing errors with ember-changeset-validations@v3, isn't it? https://github.com/kaliber5/ember-bootstrap-changeset-validations/blob/02a02e8045d8751916a49f2aaed8fc18c11cfbf7/addon/components/bs-form/element.js#L11-L14

I think it includes some bugs. If I didn't missed something it's missing a . between the key and [] for listening to changes to the array. But I'm even more confused the value of error.${property}.validation is documented as a string not as a document by ember-changeset: https://github.com/poteto/ember-changeset#error

If tried to change the code accordingly:

diff --git a/addon/components/bs-form/element.js b/addon/components/bs-form/element.js
index d5f2611..ec23664 100755
--- a/addon/components/bs-form/element.js
+++ b/addon/components/bs-form/element.js
@@ -9,7 +9,7 @@ export default BsFormElement.extend({
 
   setupValidations() {
     let key = `model.error.${this.get('property')}.validation`;
-    defineProperty(this, 'errors', computed(`${key}[]`, function() {
+    defineProperty(this, 'errors', computed(`${key}`, function() {
       return A(this.get(key));
     }));
   }

Tests are still failing. 😕

I fear Ember Changeset is not correctly notifying on changes to error.${property}.validation. But I guess we need to reproduce that upstream with a failing test before having a clear confirmation that this is the case.

[jelhan]

I fear Ember Changeset is not correctly notifying on changes to error.${property}.validation. But I guess we need to reproduce that upstream with a failing test before having a clear confirmation that this is the case.

Wrote a test for this scenario upstream: adopted-ember-addons/ember-changeset@44c46e1 It's working like expected. I fear my guess was wrong. We need a new theory what's going on. 🤔

[jelhan]

Also doesn't seem to be related to the inconsitency between ember-changeset and ember-changeset-validations in value type of error.${property}.validation. At least a test observing error.${property}.validation} is also passing for ember-changeset-validations: jelhan/ember-changeset-validations@b1da933

[BryanHunt]

I believe the correct code is:

 let key = `model.error.${this.get('property')}.validation`; 
 defineProperty(this, 'errors', computed(`${key}.[]`, function() { 
   return A(this.get(key)); 
 })); 

But that doesn't work. The fundamental problem is the computed property is only updated the first time. I posted about this on Discord and @pzuraq mentioned that ember-changeset might not be fully compatible with Octane. I don't really understand the details on the computed property changes.

[BryanHunt]

Also, to get this to work properly, I had to:

  get hasValidator() {
    return this.model != undefined && this.model.validate != undefined;
  }

[snewcomer]

@BryanHunt Do you happen to know why it might not be totally compatible with Octane?

https://github.com/poteto/ember-changeset/blob/master/addon/index.js#L60

[BryanHunt]

From my brief discussion on Discord - I don't fully understand all this:

pzuraq Last Wednesday at 11:31 AM

@Bryan take a look at the @dependentKeyCompat decorator
In general, old libraries will need to be updated before they can be used with Octane paradigms. It sounds like ember-changeset will likely need a new version.

Bryan Last Wednesday at 11:37 AM

This is the new version of ember-changeset

pzuraq Last Wednesday at 11:48 AM

looking at the code of the new version, it's really fighting autotracking and pushing an event based model
I could imagine this would cause a lot of issues with integrating with autotracking in general

@Bryan the new version doesn't appear to have autotracking integrated though yet, which makes sense, since it likely needs to continue supporting older versions of Ember
that's moreso what I meant

[snewcomer]

@BryanHunt Sorry I forgot about this when I did the major upgrades :(

My initial feeling is that since ember-changeset-validations and ember-changeset work well and are able to notify the template of changes in our new Octane world, I think this addon might be blurring the lines. I wonder if we can take a new approach to defining errors that is more in line with autotracking (and > Ember 3.13)....

[jelhan]

Ember Changeset does not support computed properties anymore for it's state: adopted-ember-addons/ember-changeset#441 😞

One possible fix is observing the original state model._errors instead of derived state model.error: 7c7249e error getter only access the _errors tracked property so there shouldn't be a difference from invalidation perspective for now. But it's a private API.

Refactor <BsForm::Element> and especially the validation logic to use tracked properties is the mid-term but nothing that we can do quickly. Especially not as there seems to be interoperability issues as soon as it comes to derived state. dependentKeyCompat should help but adds additional complexity. Not sure if we should go that path...

@simonihmig What do you think? Is this an acceptable solution for now?

@snewcomer Any concerns about using model._errors to observe changes? Of course it's a private API and you always have concerns about using private API. I'm more asking if there are foreseeable breaking changes to that one. Technically speaking Ember Changeset is also relying on that private API which belongs to Validated Changeset. 🙈

[snewcomer]

@jelhan We are at sort of a crossroads where we are interopt-ing between two different change tracking mechanisms. As a simple start, I would just make errors a plain native getter and see if that works!

get errors() {
  return A(get(this, `model.error.${this.property}.validation`));
}

I'm also not against a previous solution mentioned by Bryan, but that is a evented based mechanism, which the community as a whole will start migrating away from.

export default class BsFormElementComponent extends BsFormElement {
  @tracked errors;
  hasValidator = true;
  setupValidations() {
    const self = this;
    this.model.on('afterValidation', (key) => {
      if(key == self.property) {
        const errors = self.model.error[key];
        self.errors = errors ? errors.validation : null;
      }
    });
  }
}

[jelhan]

I agree that the native getter is the way forward. But it would require changes to the validation logic in Ember Bootstrap itself. As the plugin API for validation providers is public API that's not an easy one. Definitely requires more than just changing the computed property here to a native getter.

I fear the event based solution might introduce timing issues.

I'm tending towards using private API for now until we had the chance to refactor <BsForm::Element> to tracked properties. But waiting for @simonihmig's feedback.

[BryanHunt]

I've run into a couple of problems that may or may not be related to my solution. I've opened an issue for the first. I'm still investigating the second.

ember-bootstrap/ember-bootstrap#1017

[jelhan]

@BryanHunt Did you tested this feature branch?

[simonihmig]

Unfortunately I am not a ember-changeset user by myself IRL, so not able to contribute a lot here, other than asking these stupid questions:

• why would a native errors getter be reactive to any updates of model.error.${this.property}.validation, but not a CP?

• but if it is, @jelhan why do you think that this requires changes to the validation logic in Ember Bootstrap itself?

• As a simple start, I would just make errors a plain native getter and see if that works!

  @jelhan have you tried this?

[jelhan]

Unfortunately I am not a ember-changeset user by myself IRL, so not able to contribute a lot here, other than asking these stupid questions:

* why would a native `errors` getter be reactive to any updates of `model.error.${this.property}.validation`, but not a CP?

Due to the tracked model. This is an incompatibilty issue between tracked properties and computed properties. It's working fine together as long as the computed property observes a tracked property directly. But a computed property can't observe a derived state of a computed property aka native getter. dependentKeyCompat decorator should add a compatibility layer if used on the native getter but this would require a change on ember-changeset.

This is also the reason why computed('model.error') is not working but computed('model._errors') is working. _errors is the tracked property, error is derived state / native getter.

* but if it is, @jelhan why do you think that this requires changes to the validation logic in Ember Bootstrap itself?

* > As a simple start, I would just make errors a plain native getter and see if that works!
  
  
  @jelhan have you tried this?

Yes. It just moves the issue of native getters not notifying a computed property about the change one level up. To use this approach we would need to drop all native getters in the chain, which requires changes to <BsForm::Element>.

If no concerns are raised I tend to go with using private API as a hot fix for now. I have rebased this one after #18 has been merged to highlight how small the required change is if we use private API.

[BryanHunt]

The solution using _errors mostly works. It seems to fail when you are validating an EmberData relationship such as belongsTo. My solution has the same problem. I haven't had time to track it down. The problem is seen on the console with the following error:

Uncaught TypeError: Converting circular structure to JSON
    --> starting at object with constructor 'Store'
    |     property 'recordArrayManager' -> object with constructor 'RecordArrayManager'
    --- property 'store' closes the circle
    at JSON.stringify (<anonymous>)
    at EmberChangeset.get [as error] (validated-changeset.es5.js:1323)
    at EmberChangeset.get (index.js:220)
    at Object.get (index.js:309)
    at getPossibleMandatoryProxyValue (index.js:1722)
    at get (index.js:1788)
    at _getPath (index.js:1837)
    at get (index.js:1779)
    at BsFormElementComponent.get (observable.js:116)
    at eval (eval at <anonymous> (element.js:18), <anonymous>:1:6)

[jelhan]

It seems to fail when you are validating an EmberData relationship such as belongsTo. [...] The problem is seen on the console with the following error:My solution has the same problem. I haven't had time to track it down.

Uncaught TypeError: Converting circular structure to JSON
    --> starting at object with constructor 'Store'
    |     property 'recordArrayManager' -> object with constructor 'RecordArrayManager'
    --- property 'store' closes the circle
    at JSON.stringify (<anonymous>)
    at EmberChangeset.get [as error] (validated-changeset.es5.js:1323)
    at EmberChangeset.get (index.js:220)
    at Object.get (index.js:309)
    at getPossibleMandatoryProxyValue (index.js:1722)
    at get (index.js:1788)
    at _getPath (index.js:1837)
    at get (index.js:1779)
    at BsFormElementComponent.get (observable.js:116)
    at eval (eval at <anonymous> (element.js:18), <anonymous>:1:6)

I don't think this is related to either of our solutions or to ember-bootstrap-changeset-validations. This seems to be this upstream bug: adopted-ember-addons/validated-changeset#22 It should already be fixed in latest version of validated-changeset.

[jelhan]

Tests are failing for ember-3.13 due to emberjs/ember.js#18831. I don't expect a fix to be backported to 3.13 as it's not a long-term release. Seems to be a good reason to support only LTS releases and latest minor. Will drop support for Ember 3.13.

[ohcibi]

@BryanHunt the error about the circular json structure is fixed in validated-changeset-0.2.3 and has nothing to do with the issue about validations not showing properly.

For the matter of this issue: We had a lot of trouble with this and we are now settled with the afterValidation approach as this seems to be the right way. There is a catch though:

setupValidation is only called when the element is rendered, hence you won't receive the event when validate was called before the element got rendered. For example

// some-component-that-renders-a-bs-form-element.js

// ...
constructor() {
  this.theModelForTheElement.validate();
}
//...

This won't work as the element is rendered after the constructor and therefore setupValidation is called after the AFTER_VALIDATION event, hence the element won't get the errors property set as expected ==> element.hasErrors is false ==> the element is considered valid by ember bootstrap.

We'd need to set the initial errors if there are any when the element is rendered. However doing so in setupValidation can cause a rendering error in ember because that hook is called in the element's init method. The solution is to either change ember-bootstrap such that this hook is called in a did-insert hook or simply change the implementation of this plugin to do so. In that case we might need to add the assertions that are done in init before calling setupValidation as well.

The current solution for users of ember-changeset-validations is to add a bs-form/element component yourself and use the implementation suggested in this thread. Then if initial validation is wanted to show errors before submitting the form, make sure that this initial call to validate happens AFTER rendering the element. So either add a did-insert hook to the element itself or to the parent element (since that one fires after all child elements are inserted) but not in any constructor (or init for older ember versions).

[jelhan]

@ohcibi Can you confirm that all that complexity is not needed if we observe the private property Changeset._errors as done in this pull request? I'm asking cause I wanted to merge and release this one soon as a short-term solution.

we are now settled with the afterValidation approach as this seems to be the right way.

I disagree. The correct way forward is refactoring <BsForm::Element> to use tracked properties and have errors as a native getter instead of a computed property. I think we can do it as part of upcoming ember-bootstrap@v4.

[ohcibi]

Can you confirm that all that complexity is not needed if we observe the private property Changeset._errors as done in this pull request? I'm asking cause I wanted to merge and release this one soon as a short-term solution

Well I haven't tested whether or not the approach to observe ._errors does work yet as for our project the other approach was good enough (hence "the right way"). But as a user of this plugin I would be fine with both solutions as a short-term and if the _errors solution doesn't come with the caveat that one has to render the element before initially validating the changeset It shouldn't even affect current code that uses the plugin.

I disagree. The correct way forward is refactoring BsForm::Element to use tracked properties and have errors as a native getter instead of a computed property. I think we can do it as part of upcoming ember-bootstrap@v4.

Absolutely agreed. As I said I meant "the right way" more for the project I am working on and not for this plugin.

[snewcomer]

@jelhan

dependentKeyCompat decorator should add a compatibility layer if used on the native getter but this would require a change on ember-changeset.

I'd be happy to accept a PR on ember-changeset!

[jelhan]

@jelhan

dependentKeyCompat decorator should add a compatibility layer if used on the native getter but this would require a change on ember-changeset.

I'd be happy to accept a PR on ember-changeset!

I tried to provide such a fix but it wasn't working as expected. I guess some issues caused by overriding a getter provided by a parent class or so. I thought you are considering it as a won't fix. But to be honest I don't want to invest more time than needed into this one. Will release a stable version of Ember Bootstrap v4 hopefully within the next two months, which includes a refactor to tracked properties. So only an interim solution for a short period of time is needed.

[jelhan]

Just released v3 of this addon, which should be compatible with ember-changeset-validations@v3 out of the box. Please test it out and report bugs if you face some.