react-spectrum Storybook 7

Closes

Attempting to upgrade us to storybook 7 so that we can develop against canary since sb7 changed their UI to use the new React render API and React has now removed the old version in canary.

Steps I followed: run npx [email protected] upgrade move to new framework dedupe storybook versions in the lockfile deduped parcel deduped babel

patched some files with @devongovett 's help

Storybook now runs!

✅ Pull Request Checklist:

[ ] Included link to corresponding React Spectrum GitHub Issue.
[ ] Added/updated unit tests and storybook for this change (for new code or code which already has tests).
[ ] Filled out test instructions.
[ ] Updated documentation (if it already exists for this component).
[ ] Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

Apr 04 '24 03:04 snowystinger

so that we can develop against canary since sb7 changed their UI to use the new React render API and React has now removed the old version in canary

Sorry I just casually stumbled upon this PR, and also I'm not sure what this comment 👆 really means 😂, but I think Storybook 8 is out now? npm versions, and of course unless there's particular reason to choose v7, maybe upgrading to v8 would be better?

Apr 04 '24 14:04 sookmax

@sookmax Big emphasis on "attempting" . Upgrading majors for storybook isn't as easy as it sounds unfortunately. There usually ends up being a lot to change, and sometimes blockers. The migration assistants are really helpful, but with a large project and additional configurations, it's still a big task.

And to explain that comment, we currently run our tests in React 16, 17, 18, and canary. Running them with canary gives us a heads up on if something will break in a future release, so we can fix it earlier. In the latest canary, they've dropped support for the legacy ReactDOM.render API, so we need to switch everything to work with the newer root API, introduced in React 18. Upgrading storybook to the version that includes that will help.

Apr 04 '24 15:04 reidbarber

@reidbarber Thanks for the detailed explanation. I appreciate it!

Upgrading storybook to the version that includes that will help.

I got a little curious about what React render API Storybook uses, and found this migration guide, stating that Storybook 6.5 would use the new createRoot API if the detected React version is 18. Maybe I'm missing a lot of nuances, but aren't we already using 6.5?

Apr 04 '24 15:04 sookmax

I noticed that as well. I haven't tried running our storybook with canary, so not sure if any other issues arose when @snowystinger was debugging.

Apr 04 '24 15:04 reidbarber

Hi @sookmax thanks for your interest. The changelog is referencing how storybook renders our components, and unfortunately not how storybook renders its UI/shell. https://github.com/storybookjs/storybook/issues/18331#issuecomment-1162957861

My hope in going to 7 is that 7-8 will be a smaller jump than 6-7. We also have some working repos with parcel + storybook on v7, so we have something to compare against. storybook 8 is a bit more unknown and broke in interesting ways when I tried it.

Apr 04 '24 20:04 snowystinger

@snowystinger Thanks for taking time to explain this to me!

Forgive my ignorance, but the reason we care about what React render API the Storybook manager is using is because React as a dependency is being shared between React Aria and Storybook when testing in React 16, 17, 18, and canary? I somehow assumed Storybook would get its React separately and independently from that of React Aria, but I might be wrong. Sorry, It just a little confusing to me 😭.

My hope in going to 7 is that 7-8 will be a smaller jump than 6-7.

This sounds reasonable!

We also have some working repos with parcel + storybook on v7, so we have something to compare against.

Ah right, this definitely makes sense.

storybook 8 is a bit more unknown and broke in interesting ways when I tried it.

Haha, I haven't tried v8 myself yet, but I can see that.. I contributed to Storybook 7 a little bit last year, and well.. their issues and PRs are not very well managed, and they were breaking things too much while moving forward 😅.

Apr 05 '24 14:04 sookmax

is because React as a dependency is being shared between React Aria and Storybook when testing in React 16, 17, 18, and canary

@sookmax correct, it's a peer dependency https://github.com/storybookjs/storybook/blob/next/code/frameworks/react-webpack5/package.json#L55 & https://github.com/storybookjs/storybook/blob/next/code/renderers/react/package.json#L89 for our parcel framework as well

Apr 09 '24 11:04 snowystinger

it's a peer dependency https://github.com/storybookjs/storybook/blob/next/code/frameworks/react-webpack5/package.json#L55 & https://github.com/storybookjs/storybook/blob/next/code/renderers/react/package.json#L89 for our parcel framework as well

Yeah you're right. A framework (bundler) and a renderer (react) specifying react and react-dom as peer dependencies make sense since they are responsible for rendering user's react code in the preview (iframe) area.

If I remember correctly (I might be wrong), In Storybook 7, the manager (Storybook UI) specifies react as its devDependency and pre-bundle itself using esbuild before distribution. In fact, I think the manager cannot specify react as a peer dependency because the framework and renderer combo might not even include react, yet the manager is always a React app.

I checked out sb-7 branch and examined the installed @storybook/manager in node_modules, and it seems, in my case, one of the chunk files inside dist/ contains react in its entirety: Screenshot 2024-04-10 at 12 15 10 AM

But I think it wasn't necessarily the case with SB 6.5 now that I'm looking at node_modules/@storybook/ui/package.json in the main branch:

Anyways, I appreciate your work on the migration! It must be very challenging though.. haha.

P.S. It looks like the manager with react 18 was merged only into Storybook 8 (release-8-0) by https://github.com/storybookjs/storybook/pull/24514

Apr 09 '24 15:04 sookmax

some of these tests failures (random timeouts and jest worker being force exited) look pretty similar to the ones I've been getting in the treeview branch, perhaps meaning the babel is indeed to blame? Will try the upgrade on a branch from main

Apr 09 '24 20:04 LFDanLu

Build successful! 🎉

Apr 10 '24 00:04 rspbot

Build successful! 🎉

Apr 10 '24 00:04 rspbot

Build successful! 🎉

Apr 10 '24 04:04 rspbot

Build successful! 🎉

Apr 10 '24 21:04 rspbot

@snowystinger Still looking at the code but noticed that the storybook 17 and 16 builds are broken. The other builds (canary + 18) work just fine it seems though, controls/automated test story for Picker/themes/etc seem to be working well

Looks like it is trying to call createRoot which is only React 18+?

Apr 11 '24 00:04 LFDanLu

@snowystinger Still looking at the code but noticed that the storybook 17 and 16 builds are broken. The other builds (canary + 18) work just fine it seems though, controls/automated test story for Picker/themes/etc seem to be working well

Looks like it is trying to call createRoot which is only React 18+?

looks like problem with the shim, looking into it.

Update, I think it has to do with other react versions installed, however, setting a resolution for react did not fix the issue. I do know that it works if I remove the alias for 16 and 17, so I've done that.

Apr 11 '24 02:04 snowystinger

Build successful! 🎉

Apr 12 '24 05:04 rspbot

Build successful! 🎉

Apr 22 '24 03:04 rspbot

Build successful! 🎉

Apr 23 '24 01:04 rspbot

Build successful! 🎉

Apr 24 '24 22:04 rspbot

Build successful! 🎉

May 03 '24 01:05 rspbot

Build successful! 🎉

May 08 '24 01:05 rspbot

## API Changes

unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any', access: 'private' } unknown top level export { type: 'any', access: 'private' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'identifier', name: 'Column' } unknown top level export { type: 'identifier', name: 'Column' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown type { type: 'link' } unknown type { type: 'link' } unknown type { type: 'link' } unknown type { type: 'link' } unknown type { type: 'link' } unknown type { type: 'link' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' }

May 08 '24 01:05 rspbot

react-spectrum react-spectrum copied to clipboard

Storybook 7

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

react-spectrum
react-spectrum copied to clipboard