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

Port ngrx/entity and add createAsyncThunk #352

Merged
merged 80 commits into from
Feb 19, 2020

Conversation

markerikson
Copy link
Collaborator

@markerikson markerikson commented Feb 12, 2020

Per #76 and #333 , I've decided that there is a lot of potential benefit in having an official minimal solution for dealing with the most common data fetching and normalized data storage scenarios.

To that end, I've ported https://ngrx.io/guide/entity to Redux Toolkit, but with a bunch of changes:

  • Rewrote the internals to use Immer and simplify the logic
  • Reordered the arguments to the generated EntityAdapter methods to take state first and also feature-detect PayloadActions, so that they can be used directly as Redux case reducers

(I'd hoped that @ngrx/entity might be made more generic so it could be used by both NgRx and RTK, but the changes I ended up making are too drastic.)

I've also added a createAsyncThunk function as originally written in #76 (comment) .

This should allow usage patterns like:

const usersAdapter = createEntityAdapter();

const fetchUsers = createRequestAction(
    "users/fetch",
    () => usersAPI.fetchAll()
);

// `fetchUsers` is now a typical thunk action creator, but also has
// four more action creators attached:
// pending, fulfilled, finished, rejected
// it will automatically dispatch those based on the promise lifecycle

const usersSlice = createSlice({
    name: "users",
    initialState: usersAdapter.getInitialState(),
    reducers: {
        userAdded: usersAdapter.addOne,
        userRemoved: usersAdapter.removeOne,
        // etc
    },
    extraReducers: {
        // would also want to handle the loading state cases, probably with a state machine,
        // using the lifecycle actions attached to fetchusers
        [fetchUsers.fulfilled](state, action) {
            return usersAdapter.upsertMany(state, action.payload.result)
        }
    }
});

Published it as https://github.com/reduxjs/redux-toolkit/releases/tag/v1.3.0-alpha.0

@netlify
Copy link

netlify bot commented Feb 12, 2020

Deploy preview for redux-starter-kit-docs ready!

Built with commit 0252d80

https://deploy-preview-352--redux-starter-kit-docs.netlify.com

@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 12, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@markerikson
Copy link
Collaborator Author

I'm already not happy with how createAsyncThunk is passing in values to the generated action creators. I'd rather use prepare callbacks so that they're called like fulfilled(result, args), and we get {type: "someData/fulfilled", payload: result, meta: {args}), rather than payload: {result, args}. That way we can reuse the entity adapter methods and directly handle the results because they're in the payload.

@markerikson
Copy link
Collaborator Author

I removed the removeMany(predicate) overload since we discourage passing functions in through actions, and I don't want to have folks trying to do that when it's used as a reducer. Straightforward enough to calculate the list of IDs to remove yourself.

I also reworked the thunk actions to put the thunk args in as action.meta.args.

Unfortunately, I realized that before releasing alpha.0, my attempt to put all the fetch callback parameters into an options object probably broke the ability to correctly infer the type of the args param correctly (ie, what the user is supposed to pass into the thunk as they're dispatching it). Also, the inferred types are incredibly long and hard to read.

@phryneas , can you take a look at those issues? I'm getting over my head trying to make changes there.

@lacolaco
Copy link

lacolaco commented Feb 13, 2020

As far as I understood, I feel users/fetch doesn't seem following the redux style guide.
https://redux.js.org/style-guide/style-guide/#model-actions-as-events-not-setters

Is fetch an event? Should I choose users/fetched rather?
I'd like to know the thought about the recommendation of async actions.

@markerikson
Copy link
Collaborator Author

markerikson commented Feb 13, 2020

@lacolaco : I'm not worried about style guide stuff atm. I'm just trying to get the API working the way I want it to, and throwing together small snippets to try to exercise the code. Don't read anything into the action type names in the tests, they're not relevant or an official guidance.

@lacolaco
Copy link

@markerikson I see, thank you. Instead, I leave the same comment on the original issue.

@markerikson markerikson mentioned this pull request Feb 13, 2020
Copy link
Member

@phryneas phryneas left a comment

Choose a reason for hiding this comment

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

Okay, gave the entities a read. I like it, it stikes a nice balance. But I've annotated a few questions along the way.

@@ -0,0 +1,140 @@
import { PayloadAction } from '../createAction'

export type ComparerStr<T> = (a: T, b: T) => string
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't make sense to me. From how I read it, this will be passed to Array.prototype.sort - and the callback presented there should always return a number?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, the Array.sort() signature clearly says the result is a number (which of course should be 1/0/-1). Good catch!

Copy link

Choose a reason for hiding this comment

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

I'm late to the party, but I noticed that bug in NgRx a few weeks ago, and it has since been removed.

src/entities/sorted_state_adapter.ts Show resolved Hide resolved
src/entities/sorted_state_adapter.ts Show resolved Hide resolved
src/entities/sorted_state_adapter.ts Show resolved Hide resolved
src/entities/unsorted_state_adapter.ts Show resolved Hide resolved
src/entities/unsorted_state_adapter.ts Show resolved Hide resolved
src/entities/sorted_state_adapter.ts Show resolved Hide resolved
markerikson and others added 14 commits February 17, 2020 01:00
* prevent dispatching of further actions if asyncThunk has been cancelled, even if the payloadCreator didn't react to the `abort` request

* * add race between payloadCreator and abortedPromise
* simplify createAsyncThunk
* remove complicated logic where an AbortError thrown from the `payloadCreator` could influence the return value

* api report

* doc examples for cancellation
* Update Docusaurus and add lockfile to 43 version

* Fix lockfile

* Update netlify.toml to remove Yarn command

* Try forcing node version

Co-authored-by: Mark Erikson <mark@isquaredsoftware.com>
@github-actions
Copy link

github-actions bot commented Feb 19, 2020

Size Change: +14.9 kB (26%) 🚨

Total Size: 56.7 kB

Filename Size Change
dist/redux-toolkit.cjs.development.js 7.77 kB +3.57 kB (45%) 🚨
dist/redux-toolkit.cjs.production.min.js 3.79 kB +1.69 kB (44%) 🚨
dist/redux-toolkit.esm.js 7.58 kB +3.55 kB (46%) 🚨
dist/redux-toolkit.umd.js 27.1 kB +4.33 kB (16%) ⚠️
dist/redux-toolkit.umd.min.js 10.4 kB +1.71 kB (16%) ⚠️
ℹ️ View Unchanged
Filename Size Change
dist/index.js 149 B 0 B

compressed-size-action

- Comparer should always return a number for sorting
- Fixed missed state arg in add/remove test
- Added test to confirm expected ID change behavior
- Fixed bug in updateMany where multiple renames of one ID led to
corrupted values in entities table afterwards
@markerikson markerikson marked this pull request as ready for review February 19, 2020 04:24
@markerikson
Copy link
Collaborator Author

At this point I think I feel pretty good about the overall shape of the APIs.

I'm going to go ahead and merge this PR into the v.1.3.0-integration branch. If we do have any more tweaks, we can make them there.

Next step will be to start working on porting redux-immutable-state-invariant over.

@markerikson markerikson added this to the 1.3 milestone Feb 19, 2020
@markerikson markerikson merged commit 219be24 into v1.3.0-integration Feb 19, 2020
@markerikson markerikson deleted the feature/entities branch February 19, 2020 04:36
markerikson added a commit that referenced this pull request Mar 24, 2020
* Port ngrx/entity and add createAsyncThunk (#352)

* Initial port of `@ngrx/entity` implementation

* Remove deprecated addAll method

* Port `@ngrx/entity` tests

* Simplify immutable entity operations by wrapping with Immer

* Don't overwrite state.ids if sorting order hasn't changed

* Simplify state adapter logic using Immer

- Removed all references to DidMutate enum
- Removed unneeded logic that only checked if state was mutated

* Add `isFSA` helper to createAction

* Swap state operator order to `(state, arg)` and support FSAs

- Swapped arguments to state operators so that they can be reused
as mostly standard Redux reducers
- Added a check to handle arg as either an FSA action or a value
- Swapped argument order in all test cases
- Added one test to provide reading payload from FSAs works

* Add a test to verify adapter usage with createSlice

* Document unexpected Immer behavior with nested produce calls

* Quiet lint warnings in tests

I have no idea why the NgRx code is mutating the Array prototype
in the first place, but let's leave that there for now.

* Export Entity types as part of the public API

* Add createAsyncThunk

* Export createAsyncThunk as part of the public API

* Ignore VS Code folder

* Mark new types as alpha

* 1.3.0-alpha.0

* Remove `removeMany(predicate)` overload

* Rework dispatched thunk action contents

- Move args inside `meta`
- Include contents directly as `payload`

* Update public API types

* typings experiment

* Update createAsyncThunk tests to match API changes

* Simplify entity ID type definitions

* Add a basic request ID counter to createAsyncThunk

* Add nanoid

* Include requestId in payload creator args, and use nanoid

* Hopefully fix type definitions for empty thunk action params

- Made `ActionParams = void`, which allows not declaring any args
in the payload creation function without TS complaining
- Found out I can switch the args order back so it's `(args, other)`

* Add overloads to make EntityAdapter methods createSlice-compatible

The overloads that had `TypeOrPayloadAction<T>` were resulting in
a payload of `undefined` for the associated action creator when
passed directly as a case reducer to `createSlice`. Adding overloads
that explicitly reference `PayloadAction<T>` allows the inference
to work correctly so that action payloads are detected.

* Add a test that combines slices, async thunks, and entities

* Remove TS 3.3 and 3.4 from the Travis setup

* Update public API

* 1.3.0-alpha.1

* Rework createAsyncThunk error handling behavior

- Removed `finished` action
- Serialized `Error` objects to a plain object
- Ensured errors in `fulfilled` dispatches won't get caught wrongly
- Changed to re-throw errors in case the user wants to handle them

* Update public API

* 1.3.0-alpha.2

* createAsyncThunk return fulfilled/rejected action instead of re-… (#361)

* createAsyncThunk return fulfilled/rejected action instead of re-trowing errors

* add unwrapResult helper

* add .abort() to the createAsyncThunk thunkAction (#362)

* add .abort() to the createAsyncThunk thunkAction

* per review comments

* put `abort` on the promise returned by `dispatch(asyncThunk())`

* remove reference to DOMException

* simplify rejected action creator

* fix error==undefined case, reduce diff

* update api report

* Add initial `getAsyncThunk` API docs and usage guide

* Rename thunk types and fields and export SerializedError

* Update public API

* 1.3.0-alpha.3

* Initial fix for createAsyncThunk thunk types

* Rework `createAsyncThunk` types to enable specifying getState type

* Fix thunk test types

* Update public API

* 1.3.0-alpha.4

* manually import types to prevent a bundling issue

* strongly type slice name (#354)

* strongly type slice name

* move new generic to the end and default it to string

* use ThunkApiConfig for optional type arguments (#364)

* 1.3.0-alpha.5

* Modify createStateOperator to detect and handle Immer drafts

* Update link styling to match main Redux site

* Update blockquote styling to match main Redux site

* Update side category menu styling to match main Redux site

* Consolidate Update generic type and remove unused overload

* Update `combinedTest` based on `createStateOperator` fixes

* Add API docs for `createEntityAdapter`

* guess what time it is again - it's public API time!

* 1.3.0-alpha.6

* Remove accidental yarn.lock

* Try fixing Netlify deploys: 1

* Update DS to fix sidebar bug

* Try forcing node version

* createAsyncThunk improvements (#367)

* prevent dispatching of further actions if asyncThunk has been cancelled, even if the payloadCreator didn't react to the `abort` request

* * add race between payloadCreator and abortedPromise
* simplify createAsyncThunk
* remove complicated logic where an AbortError thrown from the `payloadCreator` could influence the return value

* api report

* doc examples for cancellation

* Remove extraneous period from abort message

* Reorder cancellation content and improve wording

* Fix code padding color busted from DS alpha.41

* 1.3.0-alpha.7

* Update Docusaurus and add lockfile to 43 version (#369)

* Update Docusaurus and add lockfile to 43 version

* Fix lockfile

* Update netlify.toml to remove Yarn command

* Try forcing node version

Co-authored-by: Mark Erikson <mark@isquaredsoftware.com>

* Try adding the compressed-size-action (#372)

* Fix potential entity bugs identified by code review

- Comparer should always return a number for sorting
- Fixed missed state arg in add/remove test
- Added test to confirm expected ID change behavior
- Fixed bug in updateMany where multiple renames of one ID led to
corrupted values in entities table afterwards

* do that public API thing

* Document caveats with update operations

Co-authored-by: Lenz Weber <mail@lenzw.de>
Co-authored-by: Thibault Gouala <thibault.gouala@gmail.com>
Co-authored-by: Alexey Pyltsyn <lex61rus@gmail.com>

* 1.3.0-alpha.8

* remove `any` where applicable (#377)

* remove `any` where applicable

* re-add `| undefined`, remove review comments

* Fork redux-devtools-extension (#384)

* Only check format for Markdown files in /docs

* Add TS port of redux-devtools extension and use it

* Remove redux-devtools-extension dependency

* Remove stray console logs from tests

* Feature/immutable invariant (#381)

* strongly type slice name (#354)

* strongly type slice name

* move new generic to the end and default it to string

* Remove accidental yarn.lock

* Update DS to fix sidebar bug

* Update Docusaurus and add lockfile to 43 version (#369)

* Update Docusaurus and add lockfile to 43 version

* Fix lockfile

* Update netlify.toml to remove Yarn command

* Try forcing node version

Co-authored-by: Mark Erikson <mark@isquaredsoftware.com>

* Try adding the compressed-size-action (#372)

* Port redux-immutable-invariant and update docs

* Update lock

* Keep immutable middleware interface types during build

* Readd lock file

* Add mention of being a fork of redux-immutable-state-invariant

* Markdown formatting

Co-authored-by: Thibault Gouala <thibault.gouala@gmail.com>
Co-authored-by: Mark Erikson <mark@isquaredsoftware.com>
Co-authored-by: Alexey Pyltsyn <lex61rus@gmail.com>

* Immutable middleware cleanup (#385)

* Inline tiny-invariant and json-stringify-safe

* Remove unused deps

* Tweak immutable middleware docs typos

* TS dep updates (#386)

* Update createEntityAdapter type reference

* Update TypeScript and Prettier to latest

* Prettier reformatting

* 1.3.0-alpha.9

* update TS docs, add new 1.3.0 APIs (#388)

* Docs: add info on how to type Meta in `createSlice`

* Docs: better example for signal.addEventListener

* docs: TS usage for `createAsyncThunk`

* docs: TS docs for `createEntityAdapter`

* Edit new TS usage docs

Co-authored-by: Mark Erikson <mark@isquaredsoftware.com>

* Clarify createAsyncThunk type handling

* Use Immer 6 alpha (#396)

* Use fork of nanoid

* Remove nanoid

* Update to Immer 6 alpha

* Enable Immer 6's ES5 support

* Add TS 3.8 coverage

* 1.3.0-alpha.10

* Formatting

* RFC createAsyncThunk: reject with typed value (#393)

* createAsyncThunk: add rejectWithValue function

* Update docs on createAsyncThunk usage, add tests for rejectWithValue, fix rejected error return

* implement AbortController stub for node, react native & IE

Co-authored-by: Matt Sutkowski <msutkowski@gmail.com>

* remove createSerializableStateInvariantMiddleware and createImmutableStateInvariantMiddleware functionality from production builds (#406)

* bump immer to v6 (#407)

* Immer 6 final (#409)

* Update to Immer 6.0.1

* Fix AbortController test

* Ignore serializability of `meta.args` by default (#410)

* 1.3.0-beta.0

* display a warning if `immutableStateInvariantMiddleware` or `ser… (#427)

* display a warning if `immutableStateInvariantMiddleware` or `serializableStateInvariantMiddleware` take too long

* Update src/utils.ts

Co-authored-by: Mark Erikson <mark@isquaredsoftware.com>

* simplify signatures of `ActionCreatorWithOptionalPayload` and `A… (#428)

* prevent any-casting of S generic on entityAdapter reducer-like f… (#436)

* prevent any-casting of S generic on entityAdapter reducer-like functions

* remove `map` from entityAdapter

* remove references to `map` from the docs

* update API report

* remove export

* Fix bug in docs for createAsyncThunk example (#417)

fetchUserById payloadCreator should fetch when in pending (not idle) state

* Feature/entity selectors (#440)

* Add a `selectById` selector

* Export Reselect types

* Update API

* 1.3.0-beta.1

* Update list of APIs and README

Co-authored-by: Lenz Weber <mail@lenzw.de>
Co-authored-by: Thibault Gouala <thibault.gouala@gmail.com>
Co-authored-by: Alexey Pyltsyn <lex61rus@gmail.com>
Co-authored-by: Lenz Weber <lenz.weber@mayflower.de>
Co-authored-by: Matt Sutkowski <msutkowski@gmail.com>
Co-authored-by: David Walsh <dlwalsh@gmail.com>
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.

9 participants