rsm-hcd / andculturecode.javascript.react Goto Github PK

Because baseUrl is defined as ./src in the tsconfig.json file, editors like VSCode will automatically import modules relative to that path instead of relative to their location.

In a normal project setting, this would be preferred - it cleans up the import pathing, and we know we're always going to be importing from under src anyway.

Nothing appears to be wrong when you're working in the context of this project, but when the Typescript is compiled down to plain JS, the imports get mucked up and modules won't resolve correctly. See rsm-hcd/AndcultureCode.JavaScript.Core#17 for an example of this.

We should remove this setting and verify the project still runs/builds as expected.

Port over the useSortAlphabetically reducer hook from one of the client projects. This issue depends on rsm-hcd/AndcultureCode.JavaScript.Core#62

A simple type alias around React's dispatch/set state action types would reduce verbosity while still getting the point across.

Something like this:

import { Dispatch, SetStateAction } from "react";

export alias type SetState<T> = Dispatch<SetStateAction<T>>;

Before:

const validate = (value: string, setError: Dispatch<SetStateAction<string>>) => {
    if (StringUtils.isEmpty(value)) {
        setError("Value cannot be empty!");
        return false;
    }

    return true;
}

const updateUser = (user: UserRecord, setContext: Dispatch<SetStateAction<UserContextRecord>>) => {
    // ... business logic ...
    setContext((prevState: UserContext) => prevState.with({ user }));
    return;
}

After:

const validate = (value: string, setError: SetState<string>) => {
    if (StringUtils.isEmpty(value)) {
        setError("Value cannot be empty!");
        return false;
    }

    return true;
}

const updateUser = (user: UserRecord, setContext: SetState<UserContextRecord>) => {
    // ... business logic ...
    setContext((prevState: UserContext) => prevState.with({ user }));
    return;
}

Open to other suggestions for naming convention, too - that was just my first thought

Updating this story as we have since downgraded back to the original version of axios (0.19.2). Troubleshoot the axios 0.21.1 upgrade and make necessary code updates (or suggest necessary code updates). Expand the original description below and see comments for additional information that may aid the investigation.

See rsm-hcd/AndcultureCode.JavaScript.Core#15 for the original issue. Since this is fixed in core now, we just need to pull in the updated package (v0.1.2)

Problem
When supplying the redirectToIfNotFound property, the application will always redirect to that location.

Solution
Atleast part of the solution is that the NestedRoutes component must have a <Switch> around its' mapped <NestedRoute> and <Redirect>.

In testing make sure your routing table is flat and nested (at least two levels - nested layouts would be even better of a test)

Assess and test high-value paths for the NestedRoutes component.

While there will be some test coverage, one clear set of tests that I see is how the component handles the redirectToIfUnauthenticated prop.

For examples of React component tests, you can reference our AndcultureCode.Javascript.React.Components library for examples using react-testing-library.

The internal project that most of this package was based off of has since evolved - we've seen the need for query params in an update service call. The signature & implementation should be updated to be more flexible for consumers. It should look something like this:

    /**
     * Creates conventional Service Update function for the supplied resource type
     * @param recordType
     * @param resourceEndpoint
     */
    update<
        TRecord extends any,
        TPathParams extends any,
        TQueryParams = undefined
    >(
        recordType: { new (): TRecord },
        resourceEndpoint: string
    ): UpdateService<TRecord, TPathParams, TQueryParams> {
        return async (
            record: TRecord,
            pathParams?: TPathParams,
            queryParams?: TQueryParams
        ) =>
            await _update<TRecord, TPathParams, TQueryParams>(
                recordType,
                record,
                resourceEndpoint,
                pathParams,
                queryParams
            );
    },

// ---------------------------------------------------------------------------------------------
// #region Private Functions
// ---------------------------------------------------------------------------------------------

const _update = async function<
    TRecord extends any,
    TPathParams extends any,
    TQueryParams extends any = any
>(
    recordType: { new (): TRecord },
    record: TRecord,
    resourceEndpoint: string,
    pathParams?: TPathParams,
    queryParams?: TQueryParams
) {
    const url = _buildUrl(record.id, resourceEndpoint, pathParams, queryParams);
    return await axios
        .put(url, record.toJS())
        .then((r) => ServiceUtils.mapAxiosResponse(recordType, r));
};

Links in the docs section that are a pointer back to a specific line of code should link to the andculture repo.

The links, for example, look something like this:

Defined in src/interfaces/authenticated-route.ts:12

As an example file: https://github.com/AndcultureCode/AndcultureCode.JavaScript.React/blob/master/docs/interfaces/authenticatedroute.md

Currently, the links point to the source of the PR merge

We've recently added service factories for batchedListService and nestedBatchedListService to a client project. These should be moved to src/services/service-factory.ts in this project. To do this, several pieces will be needed.

Types:

export type BatchedListService<TRecord, TQueryParams> = (
    queryParams: TQueryParams,
    property: keyof TQueryParams,
    size: number
) => Promise<ServiceResponse<TRecord>>;

export type NestedBatchedListService<TRecord, TPathParams, TQueryParams> = (
    pathParams: TPathParams,
    queryParams: TQueryParams,
    property: keyof TQueryParams,
    size: number
) => Promise<ServiceResponse<TRecord>>

Public Functions:

const ServiceFactory = {
    batchedList: <TRecord, TQueryParams>(
        recordType: { new (): TRecord },
        baseEndpoint: string
    ): BatchedListService<TRecord, TQueryParams> => {
        return async (
            queryParams: TQueryParams,
            property: keyof TQueryParams,
            size: number
        ) => {
            return _batchList(
                recordType,
                baseEndpoint,
                property,
                size,
                queryParams
            );
        };
    },
    nestedBatchedList: <TRecord, TPathParams, TQueryParams>(
        recordType: { new (): TRecord },
        baseEndpoint: string
    ): NestedBatchedListService<TRecord, TPathParams, TQueryParams> => {
        return async (
            pathParams: TPathParams,
            queryParams: TQueryParams,
            property: keyof TQueryParams,
            size: number
        ) => {
            return _batchList(
                recordType,
                baseEndpoint,
                property,
                size,
                queryParams,
                pathParams
            );
        };
    }
}

Private Function:

const _batchedList = async function<
    TRecord,
    TQueryParams,
    TPathParams = undefined
>(
    recordType: { new (): TRecord },
    baseEndpoint: string,
    property: keyof TQueryParams,
    size: number,
    queryParams: TQueryParams,
    pathParams?: TPathParams
) {
    const values = queryParams[property];
    if (!(values instanceof Array) || CollectionUtils.isEmpty(values)) {
        return ServiceResponseFactory.successfulList<TRecord>();
    }

    const batches = _.chunk(values, size);

    const requests = batches.map((batchValues) => {
        const batchedParams = Object.assign({}, queryParams);
        batchedParams[property] = batchValues as any;

        if (pathParams != null) {
            return AndcultureCodeServiceFactory.nestedList<
                TRecord,
                TPathParams,
                TQueryParams
            >(recordType, baseEndpoint)(pathParams, batchedParams);
        }

        return AndcultureCodeServiceFactory.list<TRecord, TQueryParams>(
            recordType,
            baseEndpoint
        )(batchedParams);
    }) as Promise<ServiceResponse<TRecord>>[];

    const results = await PromiseUtils.toArray(requests);
    return ServiceResponseFactory.successfulList(results, values.length);
};

After the _batchedList private function is added to service-factory.ts the calls to AndcultureCodeServiceFactory.nestedList and AndcultureCodeServiceFactory.list can be swapped out for a single call to the service-factory's private _list function.

Finally, a service-response-factory was added to return a successfulList result. This should also be brought over.

const ServiceResponseFactory = {
    /**
     * Default successful list response
     */
    successfulList<T>(
        resultObjects?: Array<T>,
        rowCount?: number
    ): ServiceResponse<T> {
        return {
            resultObjects: resultObjects ?? [],
            rowCount: rowCount ?? 0,
            status: 200,
        };
    },
};

The implementation in the client project has been tagged with this issue for reference.

Assess and test high-value paths for the AuthenticatedRoute component.

For examples of React component tests, you can reference our AndcultureCode.Javascript.React.Components library for examples using react-testing-library.

Use case: There is a need to add a Cache-Control header to a request to disable caching from the frontend. Rather than add a simple parameter such as skipCache, this could be plumbed through as an options object to allow more flexibility in the future. This would be needed on at least get and list calls to start with. Included options on the object for the caching property should follow the spec, see https://docs.microsoft.com/en-us/aspnet/core/performance/caching/response?view=aspnetcore-5.0

The following interfaces, records, and their associated tests should be pulled into the AndcultureCode.JavaScript.Core package, since they are not React-specific data structures or concepts.

enumerations/error-type.ts
interfaces/paged-result.ts
interfaces/result-error.ts
interfaces/result.ts
interfaces/service-response.ts
view-models/result-error-record.ts
view-models/result-record.ts

Be sure to revisit tests and ensure there is reasonable test coverage where it makes sense.

If it makes sense or is necessary, pull these utility classes into the core package as well:
utilities/collection-utils.ts
utilities/core-utils.ts

Creating an examples/samples folder would provide the opportunity to add more in depth API documentation and usage examples. Examples for #35 would be a good start.

During a recent merge & package preparation, I noticed this test failure. After a few re-runs, I noticed it on & off passing/failing. Investigate this implementation & corresponding test to ensure it is working as expected.

 FAIL  src/hooks/use-async-effect.test.tsx
  ● useAsyncEffect › isMounted equals false after cleanup

    expect(received).toBe(expected) // Object.is equality

    Expected: false
    Received: true

      80 |         await cleanup();
      81 |         await waitFor(() => expect(mockedMethod).toBeCalledTimes(1));
    > 82 |         expect(actualIsMountedValue).toBe(expectedIsMountedValue);
         |                                      ^
      83 |     });
      84 | });
      85 | 

      at src/hooks/use-async-effect.test.tsx:82:38
      at step (src/hooks/use-async-effect.test.tsx:33:23)
      at Object.next (src/hooks/use-async-effect.test.tsx:14:53)
      at fulfilled (src/hooks/use-async-effect.test.tsx:5:58)

The custom types declared for the service-factory.ts and service-hook-factory.ts were declared in the same files as the implementation that used them, but would probably make more sense to be broken out into a types folder with a consistent naming structure:

BulkUpdateService from service-factory.ts -> types/bulk-update-service-type.ts
CreateService from service-factory.ts -> types/create-service-type.ts
...
BulkUpdateServiceHook from service-hook-factory.ts -> types/bulk-update-service-hook-type.ts and so on.

Make sure these are exported from index.ts for consumer use.

Similiar to #46, the ServiceFactory.delete signature and implementation should be updated for greater flexibility.

It should look something like this:

    /**
     * Creates conventional Service Delete function for the supplied resource type
     * @param recordType
     * @param resourceEndpoint
     */
    delete<TPathParams extends any>(resourceEndpoint: string): DeleteService {
        return async (id: number, pathParams?: TPathParams) =>
            await _delete<TPathParams>(id, resourceEndpoint, pathParams);
    },

// ---------------------------------------------------------------------------------------------
// #region Private Functions
// ---------------------------------------------------------------------------------------------

const _delete = async function<TPathParams extends any>(
    id: number,
    resourceEndpoint: string,
    pathParams?: TPathParams
) {
    const url = _buildUrl(id, resourceEndpoint, pathParams);
    return await axios
        .delete(url)
        .then((r) => ServiceUtils.mapAxiosResponse(Boolean, r));
};

This would return the NetworkInformation interface from this: rsm-hcd/AndcultureCode.JavaScript.Core#101

Proposed hook could look something like this:

const useNetworkInformation = (): NetworkState => {
    const [networkState, setNetworkState] = useState<NetworkState>({
        isOnline: true,
    });

   useEffect(() => {  /* add and remove connection change handler */ }, []);

    return {
      ...networkState
    }
};

While our immediate use case is React, promise cancellation is not exclusive to this space.

See recent TravisCI build output here: https://travis-ci.org/github/AndcultureCode/AndcultureCode.JavaScript.React/builds/753115333 from PR #56

While the AndcultureCode.JavaScript.Core package was able to be upgraded to axios 0.21.1 without any known issue, it looks like typings may have changed and are breaking our ServiceFactory. Look into where the typing might be breaking down and make the necessary updates so we can get axios on the latest version, which fixes some security vulnerabilities.

See rsm-hcd/AndcultureCode.JavaScript.Core#121 for info

Issue created via and-cli

After publish, the JSDoc comments are not retained in neither the typescript declaration files or transpiled javascript. The reason being this line:
https://github.com/AndcultureCode/AndcultureCode.JavaScript.React/blob/c5d923dc395e1fcc1943ac12f91e03f5194bc9a7/tsconfig.json#L18

This is a known issue, but there still isn't a solution that allows for removing comments but still keeping the JSDoc comments. See here: microsoft/TypeScript#14619

The simplest solution for the time being is to set "removeComments": false

Looks like the same issue as before - this package should be moved to the production dependency list so it is bundled up in the build.

Uncaught Error: Cannot find module 'react-i18next'
    at webpackMissingModule (use-localization.ts:2)
    at Object../node_modules/andculturecode-javascript-react/dist/hooks/use-localization.js (use-localization.ts:2)
    at __webpack_require__ (bootstrap:784)
    at fn (bootstrap:150)
    at Object../node_modules/andculturecode-javascript-react/dist/index.js (index.ts:30)
    at __webpack_require__ (bootstrap:784)

When I attempted to switch over my project to use the published package version of this repo (I was previously referencing my local copy of the repo for development), the frontend no longer compiles due to a missing dependency rosie. This might be due to the tests being included in the final package bundle and rosie being a dev dependency.

Failed to compile.

./node_modules/andculturecode-javascript-core/dist/tests/factories/axios-response-factory.js
Module not found: Can't resolve 'rosie' in '/Users/Brandon/SpotifySync/frontend/node_modules/andculturecode-javascript-core/dist/tests/factories'

On v0.0.6 --

When we updated the exports to be export * from ... to be more concise, I think this broke my local project setup. I'm not able to reference any of the components provided by react-router-dom:

Referencing my local repo from the project's package.json seems to be fine, as it has a copy of react-router-dom installed in node_modules. However, if I copy the dist folder somewhere else and reference it there (basically the same file structure that is created installed from npm), it breaks the reference and it can't find those components.

I'm wondering if we need to switch back to named exports for forwarding vendor components/modules, or if there's a way we can wrap the whole export in an object/namespace so we access anything it provides.

Using named exports, the compiled index.js looks more like this:

var react_router_dom_1 = require("react-router-dom");
exports.BrowserRouter = react_router_dom_1.BrowserRouter;
exports.Switch = react_router_dom_1.Switch;

whereas right now it looks like this:

__export(require("react-router-dom"));

Assess and test high-value paths for the NestedRoute component.

For examples of React component tests, you can reference our AndcultureCode.Javascript.React.Components library for examples using react-testing-library.

Upgrade project's node requirement to 14.15.5, the latest LTS version. Ensure CI builds are updated to match and automated tests are passing. Just like the Typescript port of the project, we will want to make sure to regression test existing commands and maybe even release the upgrade as a beta package while issues are ironed out.

This will allow us to upgrade many of our dependencies to more recent versions that were previously limited to node >= 10, such as ts-jest and prettier off the top of my head.

Issue cloned from rsm-hcd/AndcultureCode.Cli#162 via and-cli

The usePageErrors hook correctly checks the result as an instance of a ResultRecord to pull out error messages and returns early. However, result can come from anywhere - in cases where it is an Error thrown, the array that we are returning as pageErrors is no longer a string array - it might be a [Error, string] or objects of any other shape. This can cause issues when mapping out what consumers expect to be an array of plain strings to JSX.

Add a safe guard to check for a string or otherwise try to coalesce the object into a string:

if (typeof result === "string") {
    setPageErrors((errors: string[]) => [...errors, result]);
    return;
}

setPageErrors((errors: string[]) => [...errors, result.toString()]);

Presently the various hooks for service hook factory are harder to gain access. Recommend breaking them out into 'types' folder and export individually for easier consumption.

In a recent debugging session on a client project, we found that some unnecessary state changes were being kicked off by the use-page-errors hook, specifically the resetPageErrors function that is exported. Add a simple check with the callback version of setPageErrors to check to see if the previous state has values - if it doesn't, there's no need to set the state and cause a re-render to consuming components (due to referential inequality of arrays).

Before:

const resetPageErrors = useCallback(() => {
    setPageErrors([]);
}, []);

After:

const resetPageErrors = useCallback(() => {
    setPageErrors((prevState) =>
        CollectionUtils.hasValues(prevState) ? [] : prevState
    );
}, []);

useQuery from client project.

Basically takes a record type and a params interface type as generic type arguments, re-runs the query and returns results whenever the query changes.

While the tests are passing, it looks like we're getting some warnings from the react testing library.

 PASS  src/hooks/use-window.test.tsx (10.99s)
  ● Console

    console.error
      Warning: An update to TestHook inside a test was not wrapped in act(...).
      
      When testing, code that causes React state updates should be wrapped into act(...):
      
      act(() => {
        /* fire events that update state */
      });
      /* assert on the output */
      
      This ensures that you're testing the behavior the user would see in the browser. Learn more at https://fb.me/react-wrap-tests-with-act
          in TestHook
          in Suspense

      12 |     useEffect(() => {
      13 |         const handleWindowResize = () => {
    > 14 |             setWidth(window.innerWidth);
         |             ^
      15 |             setHeight(window.innerHeight);
      16 |         };
      17 | 

      at warningWithoutStack (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:104:32)
      at warnIfNotCurrentlyActingUpdatesInDEV (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:14051:7)
      at dispatchAction (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:6467:9)
      at handleWindowResize (src/hooks/use-window.ts:14:13)
      at innerInvokeEventListeners (node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:316:27)
      at invokeEventListeners (node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:267:3)
      at EventTargetImpl._dispatch (node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:214:9)
      at EventTargetImpl.dispatchEvent (node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:87:17)

    console.error
      Warning: An update to TestHook inside a test was not wrapped in act(...).
      
      When testing, code that causes React state updates should be wrapped into act(...):
      
      act(() => {
        /* fire events that update state */
      });
      /* assert on the output */
      
      This ensures that you're testing the behavior the user would see in the browser. Learn more at https://fb.me/react-wrap-tests-with-act
          in TestHook
          in Suspense

      13 |         const handleWindowResize = () => {
      14 |             setWidth(window.innerWidth);
    > 15 |             setHeight(window.innerHeight);
         |             ^
      16 |         };
      17 | 
      18 |         const toggleEvent = (action: "add" | "remove") => {

      at warningWithoutStack (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:104:32)
      at warnIfNotCurrentlyActingUpdatesInDEV (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:14051:7)
      at dispatchAction (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:6467:9)
      at handleWindowResize (src/hooks/use-window.ts:15:13)
      at innerInvokeEventListeners (node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:316:27)
      at invokeEventListeners (node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:267:3)
      at EventTargetImpl._dispatch (node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:214:9)
      at EventTargetImpl.dispatchEvent (node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:87:17)

We should do a second pass on these and ensure we're properly calling the hook we're testing, or just getting lucky.

After some recent debugging during the attempted axios upgrade, and looking at other packages for reference, @wintondeshong and I are suspecting that the current dependencies list is incorrect, and most (if not all) of them should be listed as peerDependencies and devDependencies. This leaves it up to the consumer to install them if they are using external libraries we are referencing, such as axios, react, react-dom, etc, while maintaining copies for local development.

This should alleviate potential issues with multiple versions of the same package, ie having multiple instances of axios configured differently, or React errors from running multiple instances. It should also remove the need to re-export react-router-dom components, which was a workaround solution to use our routing abstraction in another project.

In addition to specifying these external libraries as peer dependencies, we should lighten the version requirement on them so consumers are not locked in to just the specific version of the package we are using for development, which is rigid and can cause dependency update nightmare. Use your best judgement for specifying a range or minimum version requirement based on the versions we're currently pulling in. This may need to be tweaked as the new package structure rolls out.

Because this is a pretty major change to the way the package is being used currently, we'll want to make sure we update the readme to reflect this new behavior.

Issue cloned from rsm-hcd/AndcultureCode.JavaScript.Core#95 via and-cli

Feature request

What is the expected behavior?
To have the default branch named main instead of master

What is motivation or use case for adding/changing the behavior?
Removing terminology in technology evocative of racial inequality.

How should this be implemented in your opinion?
Following Scott Hanselman's advice

1. Move master to main (maintains branch history)

git branch -m master main
git push -u origin main

2. Update the default branch in repo settings from master to main.
3. Repoint HEAD, and delete master branch.

git symbolic-ref refs/remotes/origin/HEAD refs/remotes/origin/main
git push origin --delete master

Are you willing to work on this yourself?
no