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

Conversation

NullVoxPopuli
Copy link

@NullVoxPopuli NullVoxPopuli commented Jul 16, 2023

I couldn't get the app to boot, gettign an error, not sure why -- will look after emberconf

I only minimally tried to debug

[NodeWrapper:0 /home/nvp/Development/OpenSource/ember-data-collections-demo/app] is not a SourceNode


Stack Trace and Error Report: /tmp/error.dump.17cd729144982ef406dc502e16c7d807.log
 ELIFECYCLE  Command failed with exit code 1.

^
turns out that's resolved by renaming the directory that the repo is checked out in to not have ember-data in the path

I left some notes scattered here and there.

Main things:

  • all the state involved with async wasn't captured, and there is still more to do, but I added some state to the DataWrapper to capture that
  • depending on the needs of book list, the apis could still change
  • the api used for calling the resource is no different than if you were to use a vanilla JS class
    • and using a vanilla JS class separately could still be a good idea.
      • though, you would need to wire up owner and destruction, which is what @link could be used for (unrelated, but in ember-resources util -- not used for resources)
      • vanilla JS classes would not compose as nicely as resources with other resources -- due to ownership and destruction linking --- but would be a fine api with all that abstracted


// 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

* 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


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


// 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?

@@ -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;
    });
  }
);

@gilest
Copy link

gilest commented Jul 17, 2023

I couldn't get the app to boot, gettign an error, not sure why -- will look after emberconf

^ turns out that's resolved by renaming the directory that the repo is checked out in to not have ember-data in the path

emberjs/data/issues/8606

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants