Add types and data store

[NullVoxPopuli]

Not sure how big to make this PR.
But my goal with this is to increase the DX with orbit + react.

[codecov-io]

Codecov Report

Merging #16 into master will increase coverage by 0.09%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #16      +/-   ##
==========================================
+ Coverage   92.54%   92.63%   +0.09%     
==========================================
  Files           3        3              
  Lines         161      163       +2     
  Branches       32       33       +1     
==========================================
+ Hits          149      151       +2     
  Misses         12       12

Impacted Files	Coverage Δ
src/components/DataProvider.js	100% <100%> (ø)	⬆️
src/components/withData.js	93.1% <100%> (+0.04%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b4f892...50e6761. Read the comment docs.

[hongaar]

Hi @NullVoxPopuli,

I'm not very familiar with TypeScript myself, but I'm happy to ship types with this package. Should the types be included in the bundles/transpiled directories? Would we need to modify any build script or are types included in the npm package when creating a new version?

What is the rationale for adding the sources as a convenience prop? Could you open a separate PR for this and remove it from this one to keep commits cleaner and make review easier?

Thanks for your PR!

[NullVoxPopuli]

Should the types be included in the bundles/transpiled directories?
nope, ts, by default checks the root directory of a node_module folder for types :)

Would we need to modify any build script or are types included in the npm package when creating a new version?

nope, what is here works! (I'm using this branch over at https://github.com/sillsdev/appbuilder-portal/tree/master/source/SIL.AppBuilder.Portal.Frontend/src)

What is the rationale for adding the sources as a convenience prop?

If I have filters, sorting, or pagination props that differ from my backend, I need to be able to retrieve exactly what is returned from the backend. By default store is the in-memory story, which sync to a remote source. to get the backend data directly, I need to be making requests from the remote source. So, maybe this has something to do with how I've configured my provider:

import { Bucket } from '@orbit/core';
import Orbit, { Source } from '@orbit/data';
import Store from '@orbit/store';

import LocalStorageBucket from '@orbit/local-storage-bucket';
import IndexedDBBucket, { supportsIndexedDB } from '@orbit/indexeddb-bucket';
import Coordinator, { SyncStrategy, RequestStrategy, EventLoggingStrategy } from '@orbit/coordinator';
import JSONAPISource, { JSONAPISerializer } from '@orbit/jsonapi';
import IndexedDBSource from '@orbit/indexeddb';


import { api as apiEnv, app as appEnv } from '@env';

import { schema, keyMap } from './schema';
import authedFetch, { defaultHeaders } from '@lib/fetch';

const BucketClass = (supportsIndexedDB ? IndexedDBBucket : LocalStorageBucket);

class CustomJSONAPISerializer extends JSONAPISerializer {
  // remoteId is used to track the difference between local ids and the
  // real id of the server.  This is done so that orbit can maintain
  // relationships before persisting them to the remote host.
  // (before persisting, there are no known ids)
  //
  // resourceKey just defines what local key is used for the id
  // received from the server
  //
  // remoteIds will be set when the JSONAPISource receives records
  resourceKey(type: string) { return 'remoteId'; }
}

export const serializer = new JSONAPISerializer({ schema, keyMap });

// DEBUG!
// Orbit.fetch = (...args) => {
//   console.log(args);
//   return fetch(...args);
// };

export function defaultOptions() {
  return {
    sources: {
      remote: {
        settings: {
          ...defaultSourceOptions()
        }
      }
    }
  };
}

export function defaultSourceOptions() {
  return {
    headers: {
      ...defaultHeaders()
    }
  };
}

export async function createStore() {
  // const bucket = new BucketClass({ namespace: 'scriptoria-bucket' });
  const inMemory = new Store({
    // bucket,
    keyMap,
    schema,
    name: 'inMemory'
  });


  const baseUrl = `http://${apiEnv.host || ''}/api`;

  const remote = new JSONAPISource({
    keyMap,
    schema,
    name: 'remote',
    host: baseUrl,
    SerializerClass: CustomJSONAPISerializer,
    defaultFetchSettings: {
      headers: {
        Accept: 'application/vnd.api+json',
        // these should be overwritten at runtime
        Authorization: 'Bearer not set',
        Organization: 'Org Id not set'
      }
    }
  });

  // For later when we want to persist between refreshes
  // or queue offline things
  // const backup = new IndexedDBSource({
  //   keyMap,
  //   bucket,
  //   schema,
  //   name: 'backup',
  //   namespace: 'scriptoria'
  // });

  // We don't want to have to query the API everytime we want data
  this.coordinator = new Coordinator({
    sources: [
      // backup,
      inMemory,
      remote
    ]
  });

  // TODO: when there is a network error:
  // https://github.com/dgeb/test-ember-orbit/blob/master/app/data-strategies/remote-push-fail.js


  // Pull query results from the server
  this.coordinator.addStrategy(new RequestStrategy({
    name: 'inMemory-remote-query-pessimistic',
    source: 'inMemory',
    on: 'beforeQuery',
    target: 'remote',
    action: 'pull',
    blocking: true,

    filter(query) {
      const options = ((query || {}).options || {});
      const keep = !(options.devOnly || options.skipRemote);

      return keep;
    }
  }));

  // Push updates to the server
  this.coordinator.addStrategy(new RequestStrategy({
    name: 'inMemory-remote-update-pessimistic',
    source: 'inMemory',
    on: 'beforeUpdate',
    target: 'remote',
    action: 'push',
    blocking: true,

    filter(query) {
      const options = ((query || {}).options || {});
      const keep = !(options.devOnly || options.skipRemote);

      return keep;
    }
  }));


  // sync all remote changes with the inMemory store
  this.coordinator.addStrategy(new SyncStrategy({
    source: 'remote',
    target: 'inMemory',
    blocking: true
  }));

  // this.coordinator.addStrategy(new SyncStrategy({
  //   source: 'inMemory',
  //   target: 'backup',
  //   blocking: true
  // }));

  // this.coordinator.addStrategy(new EventLoggingStrategy({
  //   sources: ['remote', 'inMemory']
  //   // sources: ['inMemory']
  // }));


  // // If there is data already stored locally, throw it in memory
  // backup.pull(q => q.findRecords())
  //   .then(transform => {
  //     console.log(transform);
  //     return inMemory.sync(transform)
  //   })
  //   .then(() => this.coordinator.activate());

  await this.coordinator.activate();

  return { store: inMemory, sources: { remote, inMemory } };
}

import * as React from 'react';
import { DataProvider } from 'react-orbitjs';
import Store from '@orbit/store';
import Coordinator from '@orbit/coordinator';

import PageLoader from '@ui/components/loaders/page';

import { createStore } from './store';
import { Source } from '@orbit/data';

interface IState {
  store: Store;
  sources: { [sourceName: string]: Source }
}

export default class APIProvider extends React.Component<{}, IState> {
  state = { store: undefined, sources: undefined };
  coordinator: Coordinator;

  constructor(props) {
    super(props);

    this.initDataStore();
  }

  async initDataStore() {
    const { sources } = await createStore();

    this.setState({ store: sources.inMemory, sources });
  }

  render() {
    const { store, sources } = this.state;

    if (!store) { return <PageLoader />; }

    return (
      <DataProvider dataStore={store} sources={sources}>
        {this.props.children}
      </DataProvider>
    );
  }
}

but idk -- I think limiting what people have access to in the way that has just causes problems -- esp when - with orbit - you can make queries on each of the sources... and that may be intentional, as it is in my case :)

Could you open a separate PR for this and remove it from this one to keep commits cleaner and make review easier?

yeah, there isn't much to this PR though, most of it is just the re-built files.

[hongaar]

Thanks so much for your effort to create this PR. I'm not very familiar with typescript myself but would love to learn more about it.

As a general remark: can you remove the new feature which adds the sources convenience prop and open a new PR for this (I'm still not sure why we need it).

Thanks!

[hongaar]

should we add @orbit/store as a (peer) dependency?

[hongaar]

(see above comment)

[hongaar]

Just out of curiosity: why is the setState() definition here?

[NullVoxPopuli]

The most annoying thing about the react + typescript combo is how react needs to identify classes / components. I actually super hate this behavior.

Basically, sometimes there is a requirement on a component to be stateful, when it doesn't matter for your interaction with that component. I feel like the interfaces implemented in the type reacts defs are maybe incorrect

[NullVoxPopuli]

HoCs are THE most annoying thing to type out. :(

[hongaar]

Would it be possible to reference the upstream Store.query function here instead of copying its definition?

[NullVoxPopuli]

Maybe! I'll give it a go

[NullVoxPopuli]

@hongaar this is a much smaller PR now :)

[hongaar]

I've given this some thought, and I don't think passing down the sources as proposed should be a responsibility of this package.

It would be fairly trivial though to pass it down with a custom wrapper, context or hoc.

Accessing the dataStore is already possible with child context, and I'm not sure if we really need this as a convenience prop either (I want to be conservative with what we pass down as to not clutter the props the wrapped component is receiving too much).

If you have additional use cases or new insights, feel free to add comments and I'll consider re-opening the PR.

[NullVoxPopuli]

I disagree, but that's fine :)

I uses dataStore everywhere, as it provides more options. I can query the network, or the cache.
I've opted not to use queryStore and updateStore

So, it'd be even more conservative to only have dataStore :)

Do you have an example off-hand of how to access the dataStore with the existing code?

for the sources, I specifically needed access to the remote store/api so I could manually interact with the response records because my filtering and sorting uses different keywords and tokens than orbit does.

This is a snippet from my custom query HoC,

        const { dataStore, sources: { remote } } = this.props;
        const querier = useRemoteDirectly ? remote : dataStore;

        const responses = {};
        const requestPromises = Object.keys(result).map(async (key: string) => {
          // ... logic!
        });

        try {
          await Promise.all(requestPromises);
        } catch (e) {
          console.error(responses, e);
          this.setState({ error: e });
        }

found here: https://github.com/sillsdev/appbuilder-portal/blob/master/source/SIL.AppBuilder.Portal.Frontend/src/data/query.tsx

and then usage:

export function withNetwork<TWrappedProps>(options: IOptions = {}) {
  const { all } = options;

  const isWantingAllProjects = all === true;

  return WrappedComponent => {
    function mapNetworkToProps(passedProps: TWrappedProps & IProps) {
      const {
        applyPagination, currentPageOffset, currentPageSize,
        applyFilter, filters, sortProperty, currentOrganizationId,
        applySort,
      } = passedProps;

      const requestOptions = buildOptions({
        include: ['organization,group,owner,products.product-definition.type']
      });

      if (isWantingAllProjects) {
        delete requestOptions.sources.remote.settings.headers.Organization;
      }

      return {
        cacheKey: [
          currentOrganizationId,
          sortProperty, filters,
          currentPageOffset, currentPageSize
        ],
        projects: [
          q => {
            let builder = q.findRecords(PROJECT);

            // these are query bilder things, but they are constructed for the backend
            // and would yield 0 results if I let orbit also filter, sort, and paginate
            if (applyFilter) { builder = applyFilter(builder); }
            if (applyPagination) { builder = applyPagination(builder); }
            if (applySort) { builder = applySort(builder); }

            return builder;
          },
          requestOptions
        ]
      };
    }

    return compose(
      query(mapNetworkToProps, { passthroughError: true, useRemoteDirectly: true }),
    )(WrappedComponent);
  };
}

so, direct access to the remote is not something I can avoid. :)

[hongaar]

Consuming the dataStore and sources with React context:

const {
  Provider: CustomDataProvider,
  Consumer: DataConsumer
} = React.createContext();

class APIProvider {
  // ...

  render() {
    const { store, sources } = this.state;

    return (
      <CustomDataProvider value={ store, sources }>
        <DataProvider dataStore={store}>
          {this.props.children}
        </DataProvider>
      </CustomDataProvider>
    );
  }
}

export DataConsumer

Then, in concrete implementation:

<DataConsumer>
  {{ store, sources } => /* render something */}
</DataConsumer>

And of course usage of DataConsumer can be included in a custom hoc for easy access to the context through props.

[NullVoxPopuli]

so, given this pattern, I wouldn't necessarily need react-orbitjs as it is now.
hm. I must ponder this.

Thanks for the example!

[hongaar]

Saved you some bytes 😄

So, react-orbitjs was never designed to be just a (very basic) DI for Orbit instances but instead to hook into Orbit events and use it to re-render components automatically (make it reactive).

[NullVoxPopuli]

but instead to hook into Orbit events and use it to re-render components automatically

yeah, I've looked through all the subscribe code in the bulk of this package.
seems like it'd be tough to get right.