UI: Ember deprecation - addObject, removeObject #25952

hashishaw · 2024-03-14T16:53:17Z

This PR manages the following deprecation, which is a subset of ember-data:deprecate-array-like:

DEPRECATION: The addObject method on the class ManyArray is deprecated. Use the native array method push instead. [deprecation id: ember-data:deprecate-array-like] This will be removed in ember-data 5.0.

This deprecation caused quite a bit of refactoring, because the deprecated methods would immediately notify Ember lifecycle of the change, but Array.push and Array.splice do not. Additionally, removeObject and addObject etc automatically handled making sure duplicate entries weren't added.

With the new changes, we now need to set the tracked value to the new value with items removed or added. I used a helper we already had to help simplify the changes. There are also a few straggling instances of addObject and removeObject if you search the codebase, but those are actually happening on items of type ArrayProxy so they are still valid. I tried to add comments everywhere that happens.

Ent tests pass locally

…forcement-form (see #16470)

- only used one place (auth-form) so simplified that usage - glimmerize and refactor so that the tests work

hashishaw · 2024-03-14T16:55:52Z

ui/app/adapters/database/role.js

@@ -144,7 +144,7 @@ export default ApplicationAdapter.extend({
  async _updateAllowedRoles(store, { role, backend, db, type = 'add' }) {
    const connection = await store.queryRecord('database/connection', { backend, id: db });
    const roles = [...(connection.allowed_roles || [])];
-    const allowedRoles = type === 'add' ? addToArray([roles, role]) : removeFromArray([roles, role]);
+    const allowedRoles = type === 'add' ? addToArray(roles, role) : removeFromArray(roles, role);


I simplified the interface for the exported methods so that we don't need to wrap the params in an array

ui/app/components/keymgmt/provider-edit.js

github-actions · 2024-03-14T17:01:49Z

CI Results:
All Go tests succeeded! ✅

hashishaw · 2024-03-14T17:05:18Z

ui/app/services/csp-event.js

-    return this.events.some((e) => e.violatedDirective.startsWith('connect-src'));
-  }),
+export default class CspEventService extends Service {
+  @tracked connectionViolations = [];


Full list of events didn't seem to be used anywhere, and the alias for the getter wasn't updating on the auth-form correctly so I refactored this service to track connectionViolations directly

hashishaw · 2024-03-14T17:14:16Z

ui/lib/core/addon/utils/search-select-has-many.js

This util was only used in one component, so I moved the logic back. See #16470

hashishaw · 2024-03-14T17:15:00Z

ui/lib/kubernetes/addon/components/page/roles.js

@@ -32,7 +32,6 @@ export default class RolesPageComponent extends Component {
    try {
      const message = `Successfully deleted role ${model.name}`;
      await model.destroyRecord();
-      this.args.roles.removeObject(model);


Destroying record automatically removes it from the store, so no need to update this.args.roles list directly

github-actions · 2024-03-19T21:59:18Z

Build Results:
All builds succeeded! ✅

hellobontempo

Nice work tackling this! Mostly just comments and cleanup questions 🏅

hellobontempo · 2024-03-19T22:28:39Z

ui/app/helpers/add-to-array.js

-export function addToArray([array, string]) {
+export function addManyToArray(array, otherArray) {
+  assert(`Both values must be an array`, Array.isArray(array) && Array.isArray(otherArray));
+  const newArray = [...array].concat(otherArray);


out of curiosity - is there a reason you used concat() here instead of spreading otherArray?

If there was, I don't remember it 😂

TIL: concat will add otherArray as a single item if it isn't an array, which is nice. https://stackoverflow.com/questions/48865710/spread-operator-vs-array-concat

Oh neat! 💡

hellobontempo · 2024-03-19T22:29:34Z

ui/app/helpers/add-to-array.js

I wonder if these helpers should live in the addon engine, it seems like something we may want access to in the wider app? It also seems like this should be a util as I don't see it used in any templates, which would simplify the extra logic needed to build the helper, but that may be too much scope creep for this PR to relocate. 😶‍🌫️

That's a good idea for a follow-on! I think it will be easier to track the changes outside of this PR

ui/app/initializers/deprecation-filter.js

ui/app/components/clients/horizontal-bar-chart.js

ui/app/components/keymgmt/provider-edit.js

ui/app/components/mfa/mfa-login-enforcement-form.js

hellobontempo · 2024-03-19T22:57:05Z

ui/app/components/secret-create-or-update.js

@@ -195,6 +195,7 @@ export default class SecretCreateOrUpdate extends Component {
    if (isBlank(item.name)) {
      return;
    }
+    // secretData is a KVObject/ArrayProxy so removeObject is fine here


thank you for the comment!

ui/app/controllers/vault/cluster/access/mfa/methods/create.js

ui/app/services/console.js

hellobontempo · 2024-03-20T00:45:11Z

ui/lib/core/addon/components/form-field.js

+    } else {
+      updatedValue = removeFromArray(updatedValue, event.target.value);
+    }
+    this.args.model[this.valuePath] = updatedValue;


I think this.setAndBroadcast handles setting the model, so I don't think line 204 is necessary?

@action setAndBroadcast(value) { this.args.model.set(this.valuePath, value); this.onChange(this.valuePath, value); }

I'm matching what the previous line 198 does (either valueArray.addObject(value) or valueArray.removeObject(value). It might be unnecessary but I'm hesitant to change such a far-reaching behavior here

🤔 Ah, to me it looks like lines 200 and 202 are adding or removing the object. I don't think 198 valueArray[method](event.target.value); updated the original model value, that appeared left to line 199, which is why the refactor appeared to duplicate that process.

hashishaw added 10 commits March 13, 2024 11:10

Update add-to-array and remove-from-array helpers

e923955

remove search-select-has-many, moved logic directly into mfa-login-en…

95bd2c2

…forcement-form (see #16470)

Replace add/remove object in MFA files - All MFA tests pass

b0d2358

Replace in PKI components (pki tests all passing)

f991430

Replace in core addon where applicable

c3a57a9

glimmerize console service -- console tests pass

de9946a

more replacements

a1ca39e

update string-list, add comment to vertical-bar-chart

a68d556

Refactor CSP Event service

1438dca

- only used one place (auth-form) so simplified that usage - glimmerize and refactor so that the tests work

small updates

5093292

hashishaw added ui pr/no-changelog labels Mar 14, 2024

hashishaw added this to the 1.17.0-rc milestone Mar 14, 2024

github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Mar 14, 2024

hashishaw commented Mar 14, 2024

View reviewed changes

ui/app/components/keymgmt/provider-edit.js Outdated Show resolved Hide resolved

hashishaw commented Mar 14, 2024

View reviewed changes

more cleanup

de5984d

hashishaw commented Mar 14, 2024

View reviewed changes

hashishaw added 2 commits March 19, 2024 16:51

Fix tests

24a8030

Remove objectAt from console-helpers

1a6659c

hashishaw marked this pull request as ready for review March 19, 2024 21:53

Merge branch 'main' into ui/VAULT-24818/deprecate-addObject

0999af1

vercel bot deployed to Preview March 19, 2024 21:59 View deployment

hellobontempo approved these changes Mar 20, 2024

View reviewed changes

hashishaw added 2 commits March 21, 2024 10:25

Address PR comments

16e6ce9

move commandIndex clearing back

4fadb8b

Merge branch 'main' into ui/VAULT-24818/deprecate-addObject

17c8712

vercel bot deployed to Preview March 21, 2024 15:57 View deployment

hashishaw and others added 2 commits March 25, 2024 13:14

Remove extra model set

d7a2b83

Merge branch 'main' into ui/VAULT-24818/deprecate-addObject

889a074

hashishaw enabled auto-merge (squash) March 25, 2024 18:21

vercel bot deployed to Preview March 25, 2024 18:26 View deployment

hashishaw merged commit 5c18a4e into main Mar 25, 2024
26 checks passed

hashishaw deleted the ui/VAULT-24818/deprecate-addObject branch March 25, 2024 18:31

hashishaw mentioned this pull request May 6, 2024

UI: upgrade Ember to 5.4 #26708

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI: Ember deprecation - addObject, removeObject #25952

UI: Ember deprecation - addObject, removeObject #25952

hashishaw commented Mar 14, 2024 •

edited

Loading

hashishaw Mar 14, 2024

github-actions bot commented Mar 14, 2024 •

edited

Loading

hashishaw Mar 14, 2024

hashishaw Mar 14, 2024

hashishaw Mar 14, 2024

github-actions bot commented Mar 19, 2024 •

edited

Loading

hellobontempo left a comment

hellobontempo Mar 19, 2024

hashishaw Mar 21, 2024

hashishaw Mar 21, 2024

hellobontempo Mar 21, 2024

hellobontempo Mar 19, 2024

hashishaw Mar 21, 2024

hellobontempo Mar 19, 2024

hellobontempo Mar 20, 2024

hashishaw Mar 21, 2024

hellobontempo Mar 21, 2024

UI: Ember deprecation - addObject, removeObject #25952

UI: Ember deprecation - addObject, removeObject #25952

Conversation

hashishaw commented Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 19, 2024 • edited Loading

hellobontempo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hashishaw commented Mar 14, 2024 •

edited

Loading

github-actions bot commented Mar 14, 2024 •

edited

Loading

github-actions bot commented Mar 19, 2024 •

edited

Loading