reduxjs / redux

A JS library for predictable global state management
https://redux.js.org
MIT License
60.93k stars 15.27k forks source link

Revamp "Redux Essentials" tutorial to be TS-first and update contents #4706

Closed markerikson closed 3 months ago

markerikson commented 6 months ago

Actual content changes for #4393 , at long last!

The current WIP example code is over in:

I'm doing another round of revisions and step-by-step checking to those code commits as I rework the tutorial content, but that should be the progression and code content I want to show off in the tutorial.

Big picture summary:

codesandbox-ci[bot] commented 6 months ago

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.

netlify[bot] commented 6 months ago

Deploy Preview for redux-docs ready!

Name Link
Latest commit 41e746beaa85980a614a26f135a4b084a87a21db
Latest deploy log https://app.netlify.com/sites/redux-docs/deploys/66abd84068ae1400081df970
Deploy Preview https://deploy-preview-4706--redux-docs.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

markerikson commented 4 months ago

THE MAIN DRAFT IS DONE!

I've finished updating the entire example app and all the content in Parts 3-8.

I do want to review Parts 1 and 2 just to see if there's anything in those worth updating. (I may want to convert the example app in Part 2 to TS as well, but that's a single sandbox.)

At this point, it should be a matter of reviewing the content changes, and then finalizing the PR:

Dan503 commented 4 months ago

I'm working through this tutorial today.

I downloaded the code here thinking it was the starter code: https://github.com/reduxjs/redux-essentials-example-app/tree/feature/tutorial-steps-ts-revamped

It has the store and everything already set up though. How do I download the starter code intended as the starting point for the tutorial?

Event the first commit in that branch has the store already set up.

markerikson commented 4 months ago

@Dan503 yeah, I need to finish preparing the branch. If you go back far enough, there should be a couple of commits labeled "SQUASHME", with the commit adding the store right after that. You'd want to make a new branch off of that commit.

On mobile atm, but let me see if I can find the right commit and paste it here.

edit

The "revamped" branch is the first attempt I did converting the repo to TS. The actual branch that shows the full steps is https://github.com/reduxjs/redux-essentials-example-app/tree/redux-essentials-ts-checked .

You're want to start from here:

Dan503 commented 4 months ago

I'll post issues here as I run into them.

I wanted to use pnpm to install the project. I ran into this error when I ran pnpm install:

$ pnpm install
 ERR_PNPM_OTHER_PM_EXPECTED  This project is configured to use yarn

pnpm install works on the version of the tutorial in master and the instructions in the docs explicitly say:

The project is configured to use Yarn 4 as the package manager, but you can use any package manager (NPM, PNPM, or Bun) as you prefer.

I'll use Yarn for now to work around the issue.

So my feedback here is to test in as many package managers as you can.

markerikson commented 4 months ago

I do have Yarn set up already, yeah, but there's nothing specific about the project that requires only Yarn.

But, sounds like PNPM tried to warn you if it detects other package manager config files.

If you delete the Yarn config file and lock file, PNPM would likely install without warnings.

(in other words, this is much more of a "PNPM tries to be helpful and warn you" problem rather than a specific issue with that repo.)

Dan503 commented 4 months ago

image

This code example is misleading as to what a reducer is. There is going to be a lot of people who will skip the previous documentation and want to just jump straight into the code tutorial so it's important not to mislead people and teach the concepts as they work through the exercises.

This code sample tells me this: "A reducer is a function for... um... retrieving a single value from the state? ...but the function just returns the exact value you pass into it.... so a reducer is a function that does nothing?"

I think it is better to just set up the store and leave a comment saying that slice reducer functions will go into the reducer property

export const store = configureStore({
  // Pass in the root reducer setup as the `reducer` argument
  reducer: {
    // Slice reducer functions will go here, more on these later
  },
})

Edit: Ok I see why you added the value function in the reducers. it was so that you can demo the Redux dev tools sooner rather than later. The "Inspecting the Redux State" section. I think this does more harm than good though and the "Inspecting the Redux State" section can just go later in the documentation when we have some real state to inspect.

Oh, and I see you also have the instruction to hover over the RootState type to see how that looks with { value: number } in there... maybe mention that later as well?

Dan503 commented 4 months ago

Creating the Posts Slice section

default export

I don't think default export should be encouraged. It makes it more difficult to import the reducer into the store.ts file since you don't have intellisense autocomplete helping you auto-import the file.

I think that line should be replaced with this:

// Export the generated reducer function as `postReducer`
export const postReducer = postSlice.reducer

Which then of course means the import in store.ts needs to be updated to

import { postsReducer } from '@/features/posts/postsSlice'

This section is also the spot where talk about the dev tools should be introduced and the RootState hover can be mentioned since we have real state in Redux to look at now.

markerikson commented 4 months ago

@Dan503 fwiw the export default someReducer is a pattern that's existed in the Redux ecosystem since 2016:

and it's something we've taught pretty consistently with RTK ever since it came out:

Agreed that default exports don't help with intellisense, and I've seen plenty of good arguments against them. If you want to do a named export like export const postsReducer = postsSlice.reducer, definitely nothing stopping you there. But this is also a pattern we've had for a while and I'm not looking to change it atm.

Dan503 commented 4 months ago

Showing the Posts List

It is important to highlight that PostList.tsx is a tsx file and not a ts file.

This is the first tsx file that the user has to create themselves while following the tutorial. I can easily see someone not noticing the x at the end of the file name, creating a PostList.ts file, then running into all sorts of confusing errors that they have no idea how to debug because they are putting JSX code in a ts file.

markerikson commented 4 months ago

@Dan503 (sorry if it sounds like I'm nitpicking or counteracting your feedback here! Just seeing the notifications come in and responding while it's on my mind.)

Might be worth saying something, but at the same time, I'm trying to keep the tutorial itself focused on Redux concepts. If you look at Page 1, I specifically added a section saying "here's a list of things you should already know before you go through this tutorial", so that I can make the assumption that "the reader is already familiar with these things" and I don't have to try to explain them. (If you take things far enough, you end up with "for this Redux tutorial, we will first explain how physics work, so we can explain how CPUs work, so we can explain assembly, all the way up to JS". Gotta draw the line somewhere! :) )

That said, I should at least add a "you should be familiar with TS syntax and TS+React basics" line to that callout block.

Dan503 commented 4 months ago

@markerikson

I get your point. At the same time, this seems like a very minimal amount of effort to cater to a larger audience and avoid unnecessary headaches.

My suggestion is to essentially add one more sentence to this paragraph:

Now that we have some posts data in our store, we can create a React component that shows the list of posts. All of the code related to our feed posts feature should go in the posts folder, so go ahead and create a new file named PostsList.tsx in there.

The new sentence being: "Note that the file has a tsx extension instead of ts as this file will contain JSX code in it."

So the full paragraph becomes:

Now that we have some posts data in our store, we can create a React component that shows the list of posts. All of the code related to our feed posts feature should go in the posts folder, so go ahead and create a new file named PostsList.tsx in there. Note that the file has a tsx extension instead of ts as this file will contain JSX code in it.

Dan503 commented 4 months ago

Adding the Single Post Route

This code block has an accessibility issue in it:

 const renderedPosts = posts.map(post => (
    <article className="post-excerpt" key={post.id}>
      <h3>{post.title}</h3>
      <p className="post-content">{post.content.substring(0, 100)}</p>

      {/* This link is an accessibility issue */}
      <Link to={`/posts/${post.id}`} className="button muted-button">
        View Post
      </Link>

    </article>
  ))

Any screen reader user tabbing through the posts will just hear "link view post, link view post, link view post" over and over with zero context around what is on the other side of that link.

To fix this accessibility issue link the title instead:

 const renderedPosts = posts.map(post => (
    <article className="post-excerpt" key={post.id}>
      <h3>
        <Link to={`/posts/${post.id}`} className="button muted-button">
          {post.title}
        </Link>
      </h3>
      <p className="post-content">{post.content.substring(0, 100)}</p>
    </article>
  ))
Dan503 commented 4 months ago

Storing Dates for Posts

The current html for time ago:

<span title={timestamp}>
    &nbsp; <i>{timeAgo}</i>
</span>

This would be a more semantically correct version of the HTML:

<time dateTime={timestamp} title={timestamp}>
    &nbsp; <i>{timeAgo}</i>
</time>

title isn't really accessible. There isn't really a quick and easy way to make the iso time accessible though unless you visibly print the ISO time stamp to the page.

Dan503 commented 4 months ago

I got up to here: https://deploy-preview-4706--redux-docs.netlify.app/tutorials/essentials/part-4-using-data#adding-user-login

I'll go through more of it tomorrow

markerikson commented 4 months ago

FWIW accessibility isn't my top priority for this tutorial - my real concerns are around actually teaching Redux concepts and usage patterns.

If I can make a couple of those tweaks I'll try, although it'll mean having to redo the entire commit stack again.

Dan503 commented 4 months ago

I like the new login section of the tutorial. It teaches a new concept at the end that the current live site tutorial doesn't cover (The using actions from different slice files aspect)

markerikson commented 4 months ago

Yep, that's the entire reason I came up with the whole login feature :) It's literally just to show off extraReducers and handling other actions, as its own concept, and separate from using extraReducers to handle thunks.

Dan503 commented 4 months ago

The current tutorial names the root redux state object RootState

When I am typing RootState and I use the intellisense auto-import feature in VS code, the default import statement it adds to the top of the file is this:

import { RootState } from '@reduxjs/toolkit/query'

Idealy it would import the local RootState type by default instead.

This ends up leading to confusion where you are getting a TS error and don't understand why, then you eventually realize that it imported the wrong RootState type.

I suggest changing the type name in the tutorial to ReduxState or AppRootState to avoid this name conflict with the RootState type in '@reduxjs/toolkit/query'.

AppRootState is probably the best name for it so that it aligns with the AppStore, AppDispatch and AppThunk types.

Dan503 commented 4 months ago

The location of the Extracting Selectors for Slices in the tutorial is very odd.

We are introducing the concept of thunks and async logic so I'm expecting this section to be all about exploring how to do async logic in Redux... then we suddenly go on a massive tangent explaining reusable selectors that have nothing to do with thunks and async logic. It's quite jarring.

I believe that the perfect place to put the reusable selectors tutorial would be when implementing the EditPostForm.tsx part of the tutorial. EditPostForm.tsx is the first time we need to reuse the selectPostById logic.

The selectPostById logic is somewhat complicated and so it is easy to justify placing the tutorial here.

Present the problem: "We need to select a post by the ID again here for the second time in our app. The logic to select a post by an ID is slightly complicated and something we are likely to need to do in multiple places throughout our app. We don't want to have to duplicate this code every time we want to select a post by its ID."

Provide the solution: "Let's use reusable selector functions in our postsSlice.tsx file to avoid writing this code over and over."

Then the Selector tutorial you have already written can slot in perfectly right there.

Edit: This also reduces the amount of refactoring that the tutorial user needs to do since the selector functions are introduced much earlier.

markerikson commented 4 months ago

Yeah, that's reasonable. I wasn't terribly thrilled about adding all the selector stuff at the beginning of Part 5. Part of my thought process was that Part 4 is already 9500 words and I was trying to keep page length somewhat balanced.

(I've also briefly considered altering the tutorial page structure to insert a "part 4.5" - put selectors and extraReducers in a page in between the existing Part 4 for data usage and Part 5 for async logic. But I also don't want to have to go reworking all the page links / invalidating existing links.)

Dan503 commented 4 months ago

The current code for selectPostById is this:

export const selectPostById = (state: RootState, postId: string) =>
  state.posts.find(post => post.id === postId)

postId is of type string.

In the SinglePostPage.tsx tutorial you retrieve postId from useParams

  const { postId } = useParams()
  const post = useAppSelector((state) => selectPostById(state, postId))

postId from useParams() is of type string | undefined. So when you refactor the const post line into this:

  const post = useAppSelector((state) => selectPostById(state, postId))

You get a TS error:

Argument of type 'string | undefined' is not assignable to parameter of type 'string'.
  Type 'undefined' is not assignable to type 'string'.

To avoid getting the TS error, I recommend postId be typed as string | undefined instead of type string.

export const selectPostById = (state: RootState, postId: string | undefined) =>
  state.posts.find(post => post.id === postId)
markerikson commented 4 months ago

I think the example code does postId! to tell TS it is defined, but yeah, that's another option.

markerikson commented 4 months ago

(btw, thank you for the very thorough review work here! Really appreciate you going through and actually thinking about what's there - very helpful!)

Dan503 commented 4 months ago

I think the example code does postId! to tell TS it is defined, but yeah, that's another option.

I haven't seen ! used in any types throughout the documentation so far. ! is also a more advanced TS concept that beginner TS users are likely to not understand. I would either avoid using that or give a quick explanation of what it does.

In the selectUserById code, you type userId as userId?: string.

export const selectUserById = (state: RootState, userId?: string) => {
  return state.users.find(user => user.id === userId)
}

I don't recommend that either. userId is a required parameter for that function. It should never be excluded when using the function.

That code allows you to write the selector like this:

const user = selectUserById(state)

That is an invalid use of the selectUserById function. Typing userId as userId: string | undefined forces people to use the selector correctly by passing in an ID (even if the ID is currently undefined).

Edit: If you make this change, remember to also update the Defining Selectors Inside of createSlice section as it has the same issue.

Dan503 commented 4 months ago

I ran into a type error with the postUpdated function.

postUpdated reducer added in the Updating Post Entries section.

The action is typed as PayloadAction<Post> for that code block (makes sense, that is all the Post has in it at this stage).

postUpdated(state, action: PayloadAction<Post>) {
    // code omitted
}

The problem comes in when we start adding new fields like date, author and reactions to the Post interface.

The code type PostUpdate = Pick<Post, 'id' | 'title' | 'content'> is introduced in the Tracking Reactions Data in Posts section.

This resolves the type conflict issue however there are two problems with it currently:

  1. The code is not highlighted even though this is the first time we see that line of code.
  2. The code that uses the PostUpdate type is omitted so the tutorial never actually tells you where to use that type in the code.

I think the date field is the first field added that breaks the ts type for the postUpdated function.

I recommend having an explicit section for fixing the type conflict on postUpdated once the date field is added to the Post interface.

Dan503 commented 4 months ago

Loading State for Requests

You have this code sample already use TS notation:

{
  // Multiple possible status enum values
  status: 'idle' | 'loading' | 'failed' | 'succeeded',
  error: string | null
}

Make it an explicit TS interface and store it somewhere global (not sure where it would make sense to store it, maybe in an apiTypes.ts file in the api directory)

export interface LoadingState {
  // Multiple possible status enum values
  status: 'idle' | 'loading' | 'failed' | 'succeeded',
  error: string | null
}

Then the PostState interface can extend the LoadingState interface to add the loading state.

interface PostsState extends LoadingState {
  posts: Post[]
}

There are multiple slice files that need to load data so it doesn't make sense to hardcode the loading state types into PostsState.

Dan503 commented 4 months ago

Another point on Loading State for Requests

The status values in the loading pattern demonstration are inconsistent.

The loading pattern demonstration has a 'loading' value.

The PostsState interface has a 'pending' value instead.

This is a bit confusing, it would be better if the values were consistent.

Dan503 commented 4 months ago

This is probably out of scope for this PR but I'll mention it anyway.

The dark-mode styling of the optional extra info sections has a couple issues.

  1. The link text is difficult to read, it is very light and it is against a light background
  2. The background color of the details box is very bright for a large dark-mode content area. It is like having a light-bulb shone in your face while working at night.

Both of these issues can be fixed with a darker background color.

image

Dan503 commented 4 months ago

Sending Data with Thunks

The features/posts/postsSlice.ts code sample has incorrect types.

The code sample in the tutorial says to write this:

export const addNewPost = createAsyncThunk(...)

To avoid a TS error, I needed to write this:

// Placed next to the PostUpdate type
type PostAddNew = Pick<Post, 'title' | 'content' | 'user'> 

// code omitted

// Added explicit <Post, PostAddNew> generics
export const addNewPost = createAsyncThunk<Post, PostAddNew>(...)
Dan503 commented 4 months ago

Another point on this section: Sending Data with Thunks

Highlight this comment line to ensure that the tutorial user reads it (I accidentally left the postAdded reducer in and ended up having it post twice)

// The existing `postAdded` reducer and prepare callback were deleted

I also suggest adding a highlighted comment line saying to delete this bit of code:

dispatch(postAdded(title, content, userId))

I also think the input fields and submit button should become disabled while the form is submitting. This prevents the user from submitting the form twice or thinking that they can modify the data before it finishes submitting.

Dan503 commented 4 months ago

Thunk Functions

I went through the entire Part 5 Async Logic and Data Fetching tutorial.

I never ended up using this anywhere in my code:

// Export a reusable type for handwritten thunks
export type AppThunk = ThunkAction<void, RootState, unknown, Action>

So there either needs to be an example added on when AppThunk should be used, or it should be removed from the tutorial.

Update

AppThunk is finally used in the Streaming Cache Updates section. The very last page of the tutorial guide.

Dan503 commented 4 months ago

Sending Login Requests to the Server

Along with that, we'll update <Navbar> and <LoginPage> to import and dispatch the new thunks instead of the previous action creators.

The <Navbar> and <LoginPage> updates need to have code samples showing what to do.

Dan503 commented 4 months ago

Simplifying Thunk Types

I'm not sure why this section is so late in the tutorial. Please don't force the reader to have to go back and do massive refactors of all their previous work unless it is required for teaching an important feature about Redux. I don't see the long winded way of writing thunks as an important Redux feature.

This should just be how thunks are introduced to the reader from the beginning. Like what you did when teaching useSelector. You did not get the reader to write all their useSelector() code and then tell them to go back and refactor it all in the next part of the tutorial. You introduced the concept of useAppSelector() from the very start so that they were able to use that method from the very beginning.

Dan503 commented 4 months ago

More on Simplifying Thunk Types

I see createAppAsyncThunk as belonging alongside useAppDispatch and useAppSelector. So instead of having useAppDispatch and useAppSelector in a hooks.ts file, I think they should all live in the withTypes.ts file.

Dan503 commented 4 months ago

This is likely out of scope for this PR...

createAppAsyncThunk appears to be able to detect the first generic quite well automatically. If a parameter is needed, the parameter generic type needs to always be explicitly declared. Unfortunately the Parameter generic is the 2nd Generic so to reach it you have to provide the return type first (the 1st Generic).

An example of what I mean:

// The only reason I am providing Post here is because I need to provide the PostAddNew type for the parameter
export const addNewPost = createAppAsyncThunk<Post, PostAddNew>(
  'posts/addNewPost',
  async (initialPost) => {
    // omitted
  },
)

What I would prefer to do:

// Post is able to be inferred while a type for the parameter is explicitly defined
export const addNewPost = createAppAsyncThunk<PostAddNew>(
  'posts/addNewPost',
  async (initialPost) => {
    // omitted
  },
)

It would be good if there was a way to make createAppAsyncThunk able to optionally accept only a parameter generic if one is needed.

Dan503 commented 4 months ago

Notifications Slice

There is a native Notification type. When using VS Code auto-import it tends to prefer just using the native Notification type instead of the custom one. Changing the name will help avoid the user accidentally using the incorrect type.

I suggest "AppNotification" as the type name.

Dan503 commented 4 months ago

Adding the Notifications List

UNKNOWN_USER is currently written like this:

const UNKNOWN_USER = {
  name: 'Unknown User'
}

I think this is a better way of handling UNKNOWN_USER since it enforces consistency with all other User values.

const UNKNOWN_USER: User = {
  name: 'Unknown User',
  id: '',
}
Dan503 commented 4 months ago

Adding the Notifications List

I did my NotificationsList component quite differently.

This is mine for comparison:

import { useAppSelector } from '../../app/withTypes'
import { TimeAgo } from '../posts/TimeAgo'
import { User, selectUserById } from '../users/usersSlice'
import { AppNotification, selectAllNotifications } from './notificationsSlice'
import { Link } from 'react-router-dom'

const UNKNOWN_USER: User = {
  name: 'Unknown User',
  id: '',
}

export function NotificationsList() {
  const notifications = useAppSelector(selectAllNotifications)

  return (
    <section className="notificationsList">
      <h2>Notifications</h2>
      {notifications.map((n) => (
        <NotificationItem notification={n} key={n.id} />
      ))}
    </section>
  )
}

interface NotificationItemProps {
  notification: AppNotification
}
function NotificationItem({ notification }: NotificationItemProps) {
  const user = useAppSelector((state) => selectUserById(state, notification.user)) || UNKNOWN_USER
  return (
    <article key={notification.id} className="notification">
      <b>{user.id ? <Link to={`/users/${user.id}`}>{user.name}</Link> : user.name}</b>
      {` ${notification.message} - `}
      <TimeAgo isoTime={notification.date} />
    </article>
  )
}

Notifications list

Dan503 commented 4 months ago

Tracking Notification Status

This should be highlighted in the code preview:

state.push(...notificationsWithMetadata)
Dan503 commented 4 months ago

Updating the Posts Slice (createEntityAdapter)

If I don't make any changes to the PostsState interface then I get this TS error on the initialState variable declaration:

Property 'posts' is missing in type 'EntityState<Post, string> & { status: "idle"; error: null; }' but required in type 'PostsState'.

The PostsState interface needs to be updated and the docs do not mention this at all. The interface is also omitted in the code sample.

This is what I updated mine to:

// src/api/api.types.ts

export interface LoadingState {
  status: LoadingStatusString
  error: string | null
}

export type LoadingStatusString = 'idle' | 'pending' | 'failed' | 'succeeded'
// src/features/posts/postsSlice.ts

import { EntityState } from '@reduxjs/toolkit'
import { LoadingState } from '../../api/api.types'

type PostsState = EntityState<Post, string> & LoadingState
Dan503 commented 4 months ago

Updating the Posts Slice (createEntityAdapter)

This code in the preview example triggers a TS error:

export const { ... } = postsAdapter.getSelectors(state => state.posts)

'state' is of type 'unknown'.

It needs to be fixed by doing this:

export const { ... } = postsAdapter.getSelectors<RootState>(state => state.posts)

It would be good if I could do createEntityAdapter.withTypes() to avoid this need. This doesn't seem to be possible at the moment though.

Dan503 commented 4 months ago

Showing Toasts for New Posts

startListening: AppStartListening in the following line of code is not used.

export const addPostsListeners = (startListening: AppStartListening) => {

The action in this line of code is also unused.

effect: async (action, listenerApi) => {
Dan503 commented 4 months ago

Showing Toasts for New Posts

The instructions currently don't say where you need to call addPostListeners(), so readers get to the end of the tutorial and it doesn't work because the function is never called.

I called it in main.ts just under the const root = createRoot(document.getElementById('root')!) line. That seemed to work.

Dan503 commented 4 months ago

I've completed all of the core Redux Essentials tutorials now and will start on the RTK tutorials tomorrow. I'm loving the new sections you have added to the tutorial. They teach the core concepts and when to use them.

EskiMojo14 commented 4 months ago

This is likely out of scope for this PR...

createAppAsyncThunk appears to be able to detect the first generic quite well automatically. If a parameter is needed, the parameter generic type needs to always be explicitly declared. Unfortunately the Parameter generic is the 2nd Generic so to reach it you have to provide the return type first (the 1st Generic).

An example of what I mean:

// The only reason I am providing Post here is because I need to provide the PostAddNew type for the parameter
export const addNewPost = createAppAsyncThunk<Post, PostAddNew>(
  'posts/addNewPost',
  async (initialPost) => {
    // omitted
  },
)

What I would prefer to do:

// Post is able to be inferred while a type for the parameter is explicitly defined
export const addNewPost = createAppAsyncThunk<PostAddNew>(
  'posts/addNewPost',
  async (initialPost) => {
    // omitted
  },
)

It would be good if there was a way to make createAppAsyncThunk able to optionally accept only a parameter generic if one is needed.

export const addNewPost = createAppAsyncThunk(
  'posts/addNewPost',
  async (initialPost: PostAddNew) => {
    // omitted
  },
)
Dan503 commented 4 months ago

Thanks @EskiMojo14

@markerikson This needs to be documented as the recommended method for using createAppAsyncThunk

export const addNewPost = createAppAsyncThunk(
  'posts/addNewPost',
  async (initialPost: PostAddNew) => {
    // omitted
  },
)
Dan503 commented 4 months ago

Prefer a video course?

Should probably mention in the text description introducing the video that the video course is written in plain JavaScript, not TypeScript.

Dan503 commented 4 months ago

Motivation

This quote textbox needs to be made dark-mode friendly. It is far too bright.

image