support Ember Bootstrap v4.5

[jelhan]

This adds support for Ember Bootstrap v4.5 while keeping support for Ember Boostrap v4.4 and before.

[jelhan]

I would recommend to use a native getter instead. But didn't wanted to change to much in this hotfix.

get _attrValidations() {
  const { model, property } = this.args;
  return get(model, `validations.attrs.${property}`);
}

Need to use get because property may be a path. We didn't decided yet if we treat as officially supported (ember-bootstrap/ember-bootstrap#1173) but changing it caused immediate bug reports in ember-bootstrap-changeset-validations (ember-bootstrap/ember-bootstrap-changeset-validations#28).

[simonihmig]

When using the getter approach, wouldn't that allow us to easily support both ember-bootstrap <4.5 and >=4.5, by something like this:

get _attrValidations() {
  const model = this.args.model ? this.args.model : this.model;
  const property ? this.args.property ? this.args.property : this.property;
  return get(model, `validations.attrs.${property}`);
}

Or would the getter not interoperate well with the CP-world used in e-b <4.5? 🤔

[jelhan]

Yes. Such a getter should support both versions.

But that could be also done even without refactoring to the getter:

    if (typeof this.args === 'object') {
      defineProperty(this, '_attrValidations', readOnly(`args.model.validations.attrs.${this.args.property}`));
    } else {
      defineProperty(this, '_attrValidations', readOnly(`model.validations.attrs.${this.property}`));
    }

[simonihmig]

Ok, so...

• if supporting all ember-bootstrap versions is as easy as it seems, I think we should do that
• when we have to touch the code anyway, I would rather do the refactoring towards a getter

[jelhan]

I tried to refactor to this getter but reactivity was not working as expected. I guess you were right that the getter is not working well with the computed properties used in Ember Bootstrap < 4.4.

[jelhan]

Added support for Ember Bootstrap v4.4 back in 379f60f. Still using the defineProperty approach. Decided to not introduce @embroider/macros but use a simple if guard as the run-time overhead should not be relevant.

[simonihmig]

Canary is failing with a strange error. Cannot see how this could be related to the changes here, but als haven't seen thiss elsewhere so far. 🤔

[jelhan]

Canary is failing with a strange error. Cannot see how this could be related to the changes here, but als haven't seen thiss elsewhere so far. thinking

I guess you are talking about this error:

            TypeError: registerDestructor is not a function
                at EmberGlimmerComponentManager.createComponent (http://localhost:7357/assets/vendor.js:71352:11)
                at http://localhost:7357/assets/vendor.js:43303:32
                at deprecateMutationsInTrackingTransaction (http://localhost:7357/assets/vendor.js:55795:9)
                at CustomComponentManager.create (http://localhost:7357/assets/vendor.js:43302:64)
                at Object.evaluate (http://localhost:7357/assets/vendor.js:50962:25)
                at AppendOpcodes.evaluate (http://localhost:7357/assets/vendor.js:49320:19)
                at LowLevelVM.evaluateSyscall (http://localhost:7357/assets/vendor.js:52884:22)
                at LowLevelVM.evaluateInner (http://localhost:7357/assets/vendor.js:52840:14)
                at LowLevelVM.evaluateOuter (http://localhost:7357/assets/vendor.js:52832:14)
                at VM.next (http://localhost:7357/assets/vendor.js:54032:24)

Have seen it in ember-bootstrap and ember-bootstrap-changeset-validations as well.

[jelhan]

I would recommend to change the GitHub Actions CI workflow on this repository to continue if ember-canary fails. See https://github.com/jelhan/create-github-actions-setup-for-ember-addon/blob/5a7f23a521b3a9f9d4555b0a557c971a20131d93/templates/.github/workflows/ci.yml#L252 as an example how that can be done.

[simonihmig]

I guess you are talking about this error:

Yes. It has been mentioned on Discord, so we can ignore this here

I would recommend to change the GitHub Actions CI workflow on this repository to continue if ember-canary fails.

Makes sense. Can you do this here? To not cause merge conflicts, as you already touched the workflow file here, and also would like to merge once the new 4.4 scenario passes.

[jelhan]

I would recommend to change the GitHub Actions CI workflow on this repository to continue if ember-canary fails.

Makes sense. Can you do this here? To not cause merge conflicts, as you already touched the workflow file here, and also would like to merge once the new 4.4 scenario passes.

Done so in 3b2171f.

Please note that the merge request is still shown with an error state even if continue-on-error is true for that scenario. As far as I know GitHub Actions does not provide a feature directly matching the allow_failures option in TravisCI.

I don't have permissions for this repository to merge myself.

[simonihmig]

Done so in 3b2171f.

Awesome, thanks your work here!

Please note that the merge request is still shown with an error state even if continue-on-error is true for that scenario. As far as I know GitHub Actions does not provide a feature directly matching the allow_failures option in TravisCI.

Yeah, I also realized this recently. This is a bit annoying, as it prevents dependabot PRs from auto-merging... 😔

I don't have permissions for this repository to merge myself.

You got mail 😉