redux icon indicating copy to clipboard operation
redux copied to clipboard

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

Open markerikson opened this issue 1 year ago • 15 comments

Actual content changes for #4393 , at long last!

The current WIP example code is over in:

  • https://github.com/reduxjs/redux-essentials-example-app/tree/feature/tutorial-steps-ts-revamped

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:

  • The entire example app is TS, start to finish
  • Improve some existing explanations
  • Cover some concepts that weren't included originally, like having multiple reducers handle one action
  • Cover newer APIs we didn't have in 2020, like createListenerMiddleware and thunks in createSlice
  • While not Redux-related, update some of the usage patterns like React Router 5 -> 6 and using uncontrolled inputs in forms instead of controlled

markerikson avatar May 12 '24 17:05 markerikson

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.

codesandbox-ci[bot] avatar May 12 '24 17:05 codesandbox-ci[bot]

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.

netlify[bot] avatar May 12 '24 17:05 netlify[bot]

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:

  • Taking the rewritten app code in https://github.com/reduxjs/redux-essentials-example-app/tree/feature/tutorial-steps-ts-checked , creating a new initial commit, and squashing any leftover intermediate commits to form the final commit sequence
  • Tagging the right commits for each page
  • Updating the CodeSandboxes to point to the tags

markerikson avatar Jun 29 '24 20:06 markerikson

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.

Dan503 avatar Jul 01 '24 00:07 Dan503

@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:

  • https://github.com/reduxjs/redux-essentials-example-app/commit/e7ce2fcbb3d90d184613f4cbec64443a14a44551

markerikson avatar Jul 01 '24 00:07 markerikson

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.

Dan503 avatar Jul 01 '24 00:07 Dan503

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.)

markerikson avatar Jul 01 '24 00:07 markerikson

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 avatar Jul 01 '24 00:07 Dan503

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.

Dan503 avatar Jul 01 '24 01:07 Dan503

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

  • https://github.com/erikras/ducks-modular-redux

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

  • https://redux.js.org/tutorials/essentials/part-2-app-structure#creating-slice-reducers-and-actions

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.

markerikson avatar Jul 01 '24 02:07 markerikson

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.

Dan503 avatar Jul 01 '24 02:07 Dan503

@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.

markerikson avatar Jul 01 '24 02:07 markerikson

@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 avatar Jul 01 '24 03:07 Dan503

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 avatar Jul 01 '24 03:07 Dan503

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 avatar Jul 01 '24 07:07 Dan503

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

Dan503 avatar Jul 01 '24 08:07 Dan503

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.

markerikson avatar Jul 01 '24 14:07 markerikson

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)

Dan503 avatar Jul 02 '24 07:07 Dan503

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.

markerikson avatar Jul 03 '24 00:07 markerikson

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 avatar Jul 03 '24 00:07 Dan503

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.

Dan503 avatar Jul 03 '24 01:07 Dan503

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.)

markerikson avatar Jul 03 '24 01:07 markerikson

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)

Dan503 avatar Jul 03 '24 01:07 Dan503

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

markerikson avatar Jul 03 '24 01:07 markerikson

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

markerikson avatar Jul 03 '24 01:07 markerikson

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 avatar Jul 03 '24 02:07 Dan503

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 avatar Jul 03 '24 02:07 Dan503

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 avatar Jul 03 '24 03:07 Dan503

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 avatar Jul 03 '24 06:07 Dan503

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 avatar Jul 03 '24 08:07 Dan503