Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 8 additions & 10 deletions app/controllers/model-types.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
/* eslint-disable ember/no-computed-properties-in-native-classes */
import Controller from '@ember/controller';
import { action, computed } from '@ember/object';
import { sort } from '@ember/object/computed';
import { action } from '@ember/object';
import { compare } from '@ember/utils';
import { inject as service } from '@ember/service';

const HIDE_EMPTY_MODELS_KEY = 'are-model-types-hidden';
Expand All @@ -16,8 +15,6 @@ export default class ModelTypesController extends Controller {

constructor() {
super(...arguments);
this.sortByNameProp = ['name'];
this.sortByDescCountProp = ['count:desc'];
}

get hideEmptyModelTypes() {
Expand All @@ -36,13 +33,14 @@ export default class ModelTypesController extends Controller {
handleSettingProperty(this.storage, ORDER_MODELS_BY_COUNT_KEY, value);
}

@sort('filtered', 'sortByNameProp')
sortByName;
get sortByName() {
return this.filtered.toSorted((a, b) => compare(a.name, b.name));
}

@sort('filtered', 'sortByDescCountProp')
sortByDescCount;
get sortByDescCount() {
return this.filtered.toSorted((a, b) => compare(b.count, a.count));
}

@computed('[email protected]', 'hideEmptyModelTypes')
get filtered() {
return this.model.filter((item) => {
let hideEmptyModels = this.hideEmptyModelTypes;
Expand Down
10 changes: 10 additions & 0 deletions app/services/storage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@ import Service, { inject as service } from '@ember/service';
import { LOCAL_STORAGE_SUPPORTED } from './storage/local';
import type LocalStorageService from './storage/local';
import type MemoryStorageService from './storage/memory';
import { tracked } from '@glimmer/tracking';

function consumeTracked(value: number): number {
return value;
}

/**
* Service that wraps either the LocalStorageService or
Expand All @@ -12,13 +17,15 @@ import type MemoryStorageService from './storage/memory';
export default class StorageService extends Service {
@service(LOCAL_STORAGE_SUPPORTED ? 'storage/local' : 'storage/memory')
declare backend: LocalStorageService | MemoryStorageService;
@tracked changed = 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@patricklx this whole system of manually updating changed to indicate something changed seems wrong to me. We should be working from derived state and use normal tracking things.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's to make local storage tracked

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just saw that you have a more sophisticated implementation:
https://github.com/adopted-ember-addons/ember-tracked-local-storage/blob/master/addon/utils/tracked-local-storage.js
:)

But maybe its not really necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and we would need for both local storage and memory storage

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just do not like manually incrementing a counter here. Perhaps you could copy over the tracked-local-storage stuff or try using something similar.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i updated it, how about now?


/**
* Reads a stored object for a give key, if any.
*
* @return {Option<String>} The value, if found
*/
getItem(key: keyof object) {
consumeTracked(this.changed);
const serialized = this.backend.getItem(key);

if (serialized === null) {
Expand All @@ -33,6 +40,7 @@ export default class StorageService extends Service {
* Store a string for a given key.
*/
setItem(key: keyof object, value: string) {
this.changed += 1;
if (value === undefined) {
this.removeItem(key);
} else {
Expand All @@ -45,6 +53,7 @@ export default class StorageService extends Service {
* Deletes the stored string for a given key.
*/
removeItem(key: keyof object) {
this.changed += 1;
this.backend.removeItem(key);
}

Expand All @@ -54,6 +63,7 @@ export default class StorageService extends Service {
* @return {Array<String>} The array of keys
*/
keys() {
consumeTracked(this.changed);
return this.backend.keys();
}
}
7 changes: 6 additions & 1 deletion tests/acceptance/data-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -91,11 +91,16 @@ module('Data Tab', function (outer) {
test('Model types are successfully listed and bound', async function (assert) {
respondWith('data:getModelTypes', {
type: 'data:modelTypesAdded',
modelTypes: getModelTypes(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unclear on how this change tests the behavior. Could you please explain?

Copy link
Collaborator Author

@patricklx patricklx Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so, the flow was the following (before this change):

  1. route waits for new models
  2. test sendMessage with initial models
  3. test if models appear on page

so there is no "update" to the page after initial render

now its like this

  1. route waits for new models
  2. test sendMessage with initial EMPTY models
  3. waits for page to load
  4. sendMessage with actual models
  5. test if models appear on page and thus checks if update works

modelTypes: [],
});

await visit('/data/model-types');

await sendMessage({
type: 'data:modelTypesAdded',
modelTypes: getModelTypes(),
});

assert.dom('.js-model-type').exists({ count: 2 });

// they should be sorted alphabetically
Expand Down