Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

First pass, extractcode from BookListComponent, not sure why app won'… #1

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions .npmrc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
resolution-mode=highest
Empty file removed app/components/.gitkeep
Empty file.
67 changes: 35 additions & 32 deletions app/components/book-list.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ import type { Collection } from '@ember-data/types';
import { query } from '@ember-data/json-api/request';
import { filterEmpty } from '@ember-data/request-utils';
import { PaginationLinks } from 'collection-demo/utils/pagination-links';
import { use, type Reactive } from 'ember-resources';
import { keepLatest } from 'ember-resources/util/keep-latest';
import { AsyncContent, CurrentPage } from './current-page';

export interface BookListSignature {
Element: HTMLDivElement;
Expand All @@ -20,19 +23,39 @@ export interface BookListSignature {
};
}

class AsyncContent<T> {
@tracked content: T | undefined;
}

export default class BookListComponent extends Component<BookListSignature> {
@service declare store: Store;
@tracked currentUrl: string | null = null;

// want owner and destruction?, use @link from ember-resources
// https://ember-resources.pages.dev/funcs/link.link
// (unrelated to resources)
// I need to RFC this in to the framework tho
links = new PaginationLinks();
dataWrapper = new AsyncContent<Collection<Book>>();

// we use this to detect inbound data changes
_firstPageOptions: { url: string } | null = null;

// exposes a .current property
//
// I don't know why TS feels the inference here isn't safe
Copy link
Author

Choose a reason for hiding this comment

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

to-do for me post-emberconf

Copy link
Author

Choose a reason for hiding this comment

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

also, why doesn't this happen in my test-apps?

currentPage: Reactive<AsyncContent<Collection<Book>>> = use(
this,
CurrentPage({
currentUrl: () => this.currentUrl,
firstPageOptions: () => this.firstPageOptions,
addPage: this.links.addPage,
})
);

// optional decorator @use
// (won't run unless accessed (which is why the current property is needed on the above)
@use currentPage2 = CurrentPage({
currentUrl: () => this.currentUrl,
firstPageOptions: () => this.firstPageOptions,
addPage: this.links.addPage,
});

@cached
get firstPageOptions(): { url: string } {
const { sort, filter, genre, author, page, limit } = this.args;
Expand All @@ -46,35 +69,15 @@ export default class BookListComponent extends Component<BookListSignature> {
return options;
}

@cached
get currentPage() {
const _firstPageOptions = this._firstPageOptions;
const firstPageOptions = this.firstPageOptions;
const currentUrl = this.currentUrl;

// if the first page options changed, we need to fetch a new first page
if (_firstPageOptions?.url !== firstPageOptions.url) {
return this.fetchPage(firstPageOptions);
}

return this.fetchPage(currentUrl ? { url: currentUrl } : firstPageOptions);
}

get books(): Collection<Book> | null {
return this.currentPage.content || null;
get currentBooks(): Collection<Book> | null {
return this.currentPage.current.content || null;
}

fetchPage(options: { url: string }) {
const dataWrapper = this.dataWrapper;
const future = this.store.request<Book>(options);

future.then((books) => {
dataWrapper.content = books.content;
this.links.addPage(books.content);
});

return dataWrapper;
}
// absorbs null-flashes as underlying data changes
Copy link
Author

Choose a reason for hiding this comment

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

null-flashes only occur because we create a new state context each time tracked data changes: https://github.com/runspired/ember-data-collections-demo/pull/1/files#diff-64e5910d87bd4744480041f7b84746de88b9f83fa6fddbfbd1127d4a51a8b391R54

we could pull that up in to the outer function and avoid that problem tho, and then we wouldn't need keeplatest

@use books = keepLatest({
value: () => this.currentBooks,
when: () => !this.currentBooks,
});

updatePage = (url: string) => {
this.currentUrl = url;
Expand Down
99 changes: 99 additions & 0 deletions app/components/current-page.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
import type Store from '@ember-data/store';
Copy link
Author

@NullVoxPopuli NullVoxPopuli Jul 16, 2023

Choose a reason for hiding this comment

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

here is what this would look like once I fix a bug with trackedFunction:

import type Store from '@ember-data/store';
import type Book from 'collection-demo/models/book';
import type { Collection } from '@ember-data/types';
import { resource, resourceFactory } from 'ember-resources';
import { TrackedFunction } from 'ember-resources/util/function';

interface FirstPageOptions {
  url: string;
  sort?: string | null;
  filter?: string | null;
  genre?: string | null;
  author?: string | null;
  page?: number | null;
  limit?: number | null;
}

export const CurrentPage = resourceFactory(
  /**
   * This way of passing args has nothing to do with resources,
   * but is just how you pass args for lazy evaluation without creating
   * an options class with its own tracked properties
   */
  (optionsFns: {
    currentUrl: () => string | null;
    firstPageOptions: () => FirstPageOptions;
    addPage: (content: Collection<unknown>) => void;
  }) => {
    let previousFirstPageOptions: FirstPageOptions | undefined;

    return resource(({ on, owner, use }) => {
      const dataWrapper = new AsyncContent<Collection<Book>>();
      const addPage = optionsFns.addPage;

      const store = owner.lookup('service:store') as unknown as Store;

      const request = use(TrackedFunction(async () => {
        const currentUrl = optionsFns.currentUrl();
        const firstPageOptions = optionsFns.firstPageOptions();

        let options = firstPageOptions;

        if (previousFirstPageOptions?.url === firstPageOptions.url) {
          options = currentUrl ? { url: currentUrl } : firstPageOptions;
        }

        const books = await store.request<Book>(options);

        return books.content;
      }));

  
      return request;
    });
  }
);

import type Book from 'collection-demo/models/book';
import { tracked } from '@glimmer/tracking';
import type { Collection } from '@ember-data/types';
import { resource, resourceFactory } from 'ember-resources';

export class AsyncContent<T> {
@tracked content: T | undefined;
@tracked isLoading = true;
@tracked error: string | Error | unknown | null = null;

get isInitiallyLoading() {
return this.isLoading && this.content === undefined;
}

get hasError() {
return this.error !== null;
}
}

interface FirstPageOptions {
url: string;
sort?: string | null;
filter?: string | null;
genre?: string | null;
author?: string | null;
page?: number | null;
limit?: number | null;
}

// TODO: inline to ember-resources?
// https://github.com/NullVoxPopuli/ember-resources/issues/945
const unwrap = <T>(x: T | (() => T)): T =>
Copy link
Author

Choose a reason for hiding this comment

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

not really needed here since we haven't implemented this resource to by type-complete from the template

typeof x === 'function' ? (x as any)() : x;

export const CurrentPage = resourceFactory(
/**
* This way of passing args has nothing to do with resources,
* but is just how you pass args for lazy evaluation without creating
* an options class with its own tracked properties
*/
(optionsFns: {
currentUrl: () => string | null;
firstPageOptions: () => FirstPageOptions;
addPage: (content: Collection<unknown>) => void;
}) => {
let previousFirstPageOptions: FirstPageOptions | undefined;

/**
* TODO: add request cancellation
* See example: https://ember-resources.pages.dev/funcs/util_remote_data.RemoteData
*/
return resource(({ on, owner, use }) => {
Copy link
Author

Choose a reason for hiding this comment

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

on is not used here, as there is no cleanup / request cancellation occurring.

use is not used here as there are no other resources being consumed -- but if Request was a resource from ember-data, that could be used

const dataWrapper = new AsyncContent<Collection<Book>>();
const currentUrl = unwrap(optionsFns.currentUrl);
const firstPageOptions = unwrap(optionsFns.firstPageOptions);
const addPage = optionsFns.addPage;

const store = owner.lookup('service:store') as unknown as Store;

async function fetchPage() {
// Don't accidentally auto-track anything on data-wrapper
// when invoking fetchPage
await Promise.resolve();

try {
dataWrapper.error = null;
dataWrapper.isLoading = true;

let options = firstPageOptions;

if (previousFirstPageOptions?.url === firstPageOptions.url) {
options = currentUrl ? { url: currentUrl } : firstPageOptions;
}

/**
* TODO: make this its own resource so it can be used directly
*
* const books = use(Request(() => options));
*
* or something like that,
* that pushes the async/await handling out of this code, and in to the request code.
*/
const books = await store.request<Book>(options);
dataWrapper.content = books.content;
addPage(books.content);
} catch (e) {
dataWrapper.error = e;
} finally {
dataWrapper.isLoading = false;
}
}

fetchPage();

return dataWrapper;
});
}
);
2 changes: 1 addition & 1 deletion app/utils/pagination-links.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export class PaginationLinks {
}

get filteredPages() {
const { pages } = this;
const { pages = [] } = this;
const filtered: { index: number; link: string }[] = [];

for (let i = 0; i < pages.length; i++) {
Expand Down
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@
},
"dependencies": {
"@ember-data/request-utils": "5.3.0-alpha.6",
"@html-next/vertical-collection": "^4.0.2"
"@html-next/vertical-collection": "^4.0.2",
"ember-resources": "^6.2.2"
}
}
Loading