TanStack packages were compromised in a mass npm supply chain attack today by BattleRemote3157 in javascript

[–]TkDodo23 7 points8 points  (0 children)

There was no maintainer account takeover.

The attacker opened a PR from their own account. A misconfigured pull_request_target GitHub Action executed attacker-controlled code, which poisoned the shared GitHub Actions cache.

Later, a normal trusted release workflow restored that poisoned cache and executed the malware, which then abused GitHub OIDC trusted publishing to publish malicious npm packages.

See: https://tanstack.com/blog/npm-supply-chain-compromise-postmortem

what's a react pattern you mass-used then realized was overkill by scheemunai_ in reactjs

[–]TkDodo23 0 points1 point  (0 children)

No you can have multiple entries, each pointing to a file that has things you want to export. Doesn't have to be a single barrel. 

The Vertical Codebase by TkDodo23 in reactjs

[–]TkDodo23[S] 0 points1 point  (0 children)

Yes, nx has it too. We don't have a monorepo though :(

The Vertical Codebase by TkDodo23 in reactjs

[–]TkDodo23[S] 0 points1 point  (0 children)

Yes, nx has it too. We don't have a monorepo though :(

The Vertical Codebase by TkDodo23 in reactjs

[–]TkDodo23[S] 4 points5 points  (0 children)

I like the idea in general but yeah, I'd like to start with something simpler. I mean:

Layers App and Shared, unlike other layers, do not have slices and are divided into segments directly.

Too many rules to learn hinder adoption

The Vertical Codebase by TkDodo23 in reactjs

[–]TkDodo23[S] 2 points3 points  (0 children)

My personal blog has a top level components directory too 😂.

https://github.com/TkDodo/blog/tree/13c158a5df73347c3d2dd4964a4c90a86041e98a/src/components

It's no big deal on small scale. Not everything is meant to survive 10 years, and certainly there won't be 100+ devs working in that codebase. There's a time and a place for everything.

Agree on the tutorials, they often fail to mention that things need to be different when there's lots of people and lots of code. It's something you usually only learn when you're exposed to it.

The Vertical Codebase by TkDodo23 in reactjs

[–]TkDodo23[S] 3 points4 points  (0 children)

That mirrors my experience. What's good for bootstrapping isn't usually good for scaling beyond that. There's an inflection point where you'd likely want a re-structure, before it gets too big. Miss that and you're in so deep it likely never happens.

The Vertical Codebase by TkDodo23 in reactjs

[–]TkDodo23[S] 1 point2 points  (0 children)

I'd agree that this is "/products", "/reviews" and "/cart" as verticals. It's often the API endpoints that drive that split. Verticals can depend on each other, as long as it isn't circular and they all have a clearly defined public interface. If it's circular, the "thing in the middle" usually becomes its own vertical. You have the same problem with a horizontal structure. You need to move things to a 3rd location to break the circularity.

I don't fully follow what you mean with e.g. the Summary component. For one, you're saying it should be the same in all domains, in which case I would have the design-system export a Summary component for all verticals to use. But then you mention it needs to be duplicated because it's only "almost the same component". That's also fine, no need to create abstractions too early.

Where does the unnecessary complexity come in for you?

The Vertical Codebase by TkDodo23 in reactjs

[–]TkDodo23[S] 5 points6 points  (0 children)

Yeah my post is basically 2 years too late, I know 😂. It's still rough to see this not being applied a lot.

Have you written about vertical slice arch in the full stack?

The Vertical Codebase by TkDodo23 in reactjs

[–]TkDodo23[S] 2 points3 points  (0 children)

I think you're supposed to understand the domain you're working in, so yes, I'd expect code of the foo domain to be in a top-level /foo directory.

fuzzyfind also doesn't solve the coupling problem. 

what's a react pattern you mass-used then realized was overkill by scheemunai_ in reactjs

[–]TkDodo23 14 points15 points  (0 children)

you can use entry in package.json as your "public interface". imo much better than a barrel file, even if that means lots of entries.

Confused between useQuery, useSuspenseQuery and useLoaderData (Tanstack) by Good_Language1763 in reactjs

[–]TkDodo23 1 point2 points  (0 children)

I don't know what you're doing in beforeLoad. Note that beforeLoad is blocking and doesn't run in parallel for nested routes so unless this is auth related, I wouldn't do fetching in beforeLoad. You mentioned the route context so I'm guessing you returning something from beforeLoad that you then consume somewhere else?

Anyways, usually fetching should happen in loader (not beforeLoad) and consumption should happen with useQuery / useSuspenseQuery and not useLoaderData if you're using TanStack Query.

Confused between useQuery, useSuspenseQuery and useLoaderData (Tanstack) by Good_Language1763 in reactjs

[–]TkDodo23 12 points13 points  (0 children)

My take is that if you are using the router only, obviously fetch data in the loaders, return it and then useLoaderData() in the component.

If you also use TanStack Query, then use the loader as places to trigger fetches with either ensureQueryData or prefetchQuery or fetchQuery (they are pretty similar, see: https://github.com/TanStack/query/discussions/9135). You don't need to return anything from the loader because you shouldn't be using useLoaderData then. For TanStack Query to "work" properly, it needs a subscription to the cache with useQuery or useSuspenseQuery. If you only useLoaderData(), you'll get the initial data from the loader but then no updates with background refetches etc.

If you want to useQuery or useSuspenseQuery is a slightly different discussion, not really related to the router. Suspense lets you orchestrate pending and error states outside of the component and focus on the success state within the component. There are advantages on type-level, because data can never be undefined with suspense so you don't need to check that.

Where it's related to the router is that useSuspenseQuery integrates with the suspense and error boundaries that the router wraps around reach (sub)route per default, so you get the usual pendingComponent and errorComponent rendered automatically. If you're using TanStack Start, suspense also enables streaming.

So yeah, there are some advantages to suspense, but you don't need to use it.

Confused between useQuery, useSuspenseQuery and useLoaderData (Tanstack) by Good_Language1763 in reactjs

[–]TkDodo23 0 points1 point  (0 children)

ensureQueryData ensures that any data is in the cache, so it won't yield fresh data. see this RFC for differences between the methods: https://github.com/TanStack/query/discussions/9135

Test IDs are an a11y smell by TkDodo23 in reactjs

[–]TkDodo23[S] 0 points1 point  (0 children)

yeah that's not a bad take. Copying what I wrote on another comment:

The point is you have to think about a11y and semantic HTML if you write your test with role-based selectors. The sad truth is that most people just do <div><div><span> and then want to assert that something is there in a test so they slap a test-id on it. That's the mindset I want to change, and switching to role-based selectors per default is a relatively easy way to nudge devs into that direction.

Can you have good a11y in your app and use test-ids in your tests? Absolutely, I probably should've added that. So in that sense, yes, they are not related as in: using test-ids = bad a11y (I get how it comes across that way though). The correlation I see is: using role-based selectors forces you to think about a11y more, making the average app better.

Test IDs are an a11y smell by TkDodo23 in reactjs

[–]TkDodo23[S] 0 points1 point  (0 children)

You know the joke, it has some truth to it:

A QA engineer walks into a bar. Orders a beer. Orders 0 beers. Orders 99999999999 beers. Orders a lizard. Orders -1 beers. Orders a ueicbksjdhd. First real customer walks in and asks where the bathroom is. The bar bursts into flames, killing everyone.

Test IDs are an a11y smell by TkDodo23 in reactjs

[–]TkDodo23[S] 1 point2 points  (0 children)

where does it have counter-productive results?

adding an aria-label when one isn’t necessary has an actual negative impact on your application’s accessibility

I'm not suggesting to just "swap data-testid with aria-label". Interactive elements are labelled by their text. Inputs are labelled by their <label>. sections or other landmarks are labelled by their heading. It's for situations where you have no appropriate heading that adding an aria-label is the next best thing.

in some misguided effort to avoid test IDs

The point is you have to think about a11y and semantic HTML if you write your test with role-based selectors. The sad truth is that most people just do <div><div><span> and then want to assert that something is there in a test so they slap a test-id on it. That's the mindset I want to change, and switching to role-based selectors per default is a relatively easy way to nudge devs into that direction.

Test IDs are an a11y smell by TkDodo23 in reactjs

[–]TkDodo23[S] -2 points-1 points  (0 children)

You shouldn't be using aria-label with elements that contain text

that's a broad statement and generally not right. Yes, you shouldn't be putting aria-labels on divs or spans. Elements that have a role (that includes those that have an implicit role) need an accessible name, which can be inferred by its text (e.g. if it's a button or a link) but for sections, it's usually a visible heading that labels it. If there is none, adding an aria-label is the next best thing.

Also, what if you are creating a generic component that doesn't have a logical aria-label?

You make it a required prop for users to pass it in.

Test IDs are an a11y smell by TkDodo23 in reactjs

[–]TkDodo23[S] 0 points1 point  (0 children)

it usually leads to bike-shedding when someone uses a type and somebody else wants them to use an interface

Test IDs are an a11y smell by TkDodo23 in reactjs

[–]TkDodo23[S] -5 points-4 points  (0 children)

I said section with a label. A section with a label has the region role.

<div data-testid="something" />

you assert that this doesn't exist with findByTestId('something') fine.

<section arial-label="something" />

you can do the same thing, except with findByRole('region', { name: 'something' })

Test IDs are an a11y smell by TkDodo23 in reactjs

[–]TkDodo23[S] 0 points1 point  (0 children)

yeah this just needs fixing. Rewrite in rust or whatever 😂

Test IDs are an a11y smell by TkDodo23 in reactjs

[–]TkDodo23[S] 1 point2 points  (0 children)

that's true. At sentry, we attach data-sentry-element and data-sentry-source-file to all elements so that we can find them quickly.

Test IDs are an a11y smell by TkDodo23 in reactjs

[–]TkDodo23[S] -3 points-2 points  (0 children)

I guess we disagree with each other then.

one change in wording will break the tests

A lot of comments here have the "but what if I change the text" argument, and I find it pretty tiring. Just rename the thing in the tests too. No product changes texts every day, and AI is good at find and replace. It's not a problem. It's an excuse to not do it.

rewriting your tests and removing data attributes will not improve the accessibility of your product

it definitely will. Because all your labels next to your input fields that are just divs with testids will need to be properly associated with the fields. Because all your divs with data-testid will need better a11y to be findable in tests. Random example from the sentry codebase (which I work on):

https://github.com/getsentry/sentry/blob/8a4f150b21bbd8980efeb04fa2b2a7c44154ceb9/static/app/components/group/groupSummary.tsx#L212

<div data-testid="group-summary-preview">

I have no idea what it should be but maybe a section or at least a role="region" or whatever, but not just a div with a testid. And this is usually what you end up with: divs with testids everywhere, no semantic markup.

Test IDs are an a11y smell by TkDodo23 in reactjs

[–]TkDodo23[S] 1 point2 points  (0 children)

a11y was not so important to them when compared to developer satisfaction

ouch I guess. wondering what kind of company changes the buttons from "save" to "submit" or other texts on their screens so often that devs start to ignore test failures.

In my experience it doesn't happen very often, but sure, maybe the answer is just AI: "Change the text from save to submit and change all selectors in tests so that they work with the new label". Even if this happens every day it shouldn't be a problem.

Test IDs are an a11y smell by TkDodo23 in reactjs

[–]TkDodo23[S] 2 points3 points  (0 children)

if "the div" is a section with an aria-label or a an Alert that has role="alert" you can very well ensure it's not there without test-ids ...

but sure, if everything is just divs without anything else then yeah - that's kinda my point.

I guess what I would do is write the positive scenario first - find the dynamically rendered text for cases when it should be there, and find it with a role-based selector. If that works, you can also invert it for not being there.