Storybook 7 #6150

snowystinger · 2024-04-04T03:49:23Z

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

sookmax · 2024-04-04T14:44:25Z

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?

reidbarber · 2024-04-04T15:02:19Z

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

sookmax · 2024-04-04T15:45:55Z

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

reidbarber · 2024-04-04T15:54:18Z

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.

snowystinger · 2024-04-04T20:25:15Z

Hi @sookmax thanks for your interest. The changelog is referencing how storybook renders our components, and unfortunately not how storybook renders its UI/shell.
storybookjs/storybook#18331 (comment)

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.

sookmax · 2024-04-05T14:54:16Z

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

snowystinger · 2024-04-09T11:34:24Z

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

sookmax · 2024-04-09T15:32:23Z

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:

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 storybookjs/storybook#24514

LFDanLu · 2024-04-09T20:03:51Z

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

rspbot · 2024-04-10T00:00:51Z

Build successful! 🎉

ktabors

Thank you for doing this! I like the seperation of chromatic and chromatic-fc. The PR does a good job of hiding the difficulty of getting the correct versions and deduplicating.

rspbot · 2024-04-10T04:24:09Z

Build successful! 🎉

rspbot · 2024-04-10T21:37:31Z

Build successful! 🎉

LFDanLu · 2024-04-11T00:17:37Z

@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+?

snowystinger · 2024-04-11T02:10:24Z

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

# Conflicts: # package.json # yarn.lock

rspbot · 2024-04-12T05:12:06Z

Build successful! 🎉

# Conflicts: # package.json # packages/dev/parcel-packager-ssg/package.json # packages/dev/parcel-resolver-build/package.json # packages/dev/parcel-resolver-docs/package.json # packages/dev/parcel-resolver-optimize-locales/package.json # packages/dev/parcel-resolver-storybook/package.json # packages/dev/parcel-transformer-docs/package.json # packages/dev/parcel-transformer-mdx-docs/package.json # yarn.lock

rspbot · 2024-04-22T03:55:30Z

Build successful! 🎉

# Conflicts: # packages/@react-spectrum/card/test/Card.test.js # packages/@react-spectrum/menu/test/SubMenuTrigger.test.tsx

rspbot · 2024-04-23T01:58:52Z

Build successful! 🎉

rspbot · 2024-04-24T22:47:45Z

Build successful! 🎉

LFDanLu

LGTM to me, storybooks seem to work and chromatic runs clean. Just some small changes that I assume were unintentional/from the merge/leftover from testing

LFDanLu · 2024-04-25T00:23:17Z

packages/@react-spectrum/checkbox/package.json

+    "@react-spectrum/layout": "^3.6.3",
    "@react-spectrum/utils": "^3.11.5",
+    "@react-spectrum/view": "^3.6.8",


Where did these new deps come from? Seems to come up in a couple of the package.json changes, were they auto added by the IDE perhaps? The only imports for these are in the stories and stuff so we don't need them here right?

LFDanLu · 2024-04-25T00:33:04Z

.eslintignore

+packages/*/*/i18n
 packages/react-aria/dist
+packages/react-aria/i18n
 packages/react-aria-components/dist
+packages/react-aria-components/i18n


not sure this was intentional or not?

It was, they need to be added to our clean function as well.
I think they came from build, possibly in another branch. Either way, they need to be ignored and/or removed by clean_all

ah I misunderstood, I thought this was ignoring the i18n files like useFilter, makes sense

# Conflicts: # package.json # packages/@react-spectrum/actionbar/package.json # packages/@react-spectrum/actiongroup/package.json # packages/@react-spectrum/avatar/package.json # packages/@react-spectrum/badge/package.json # packages/@react-spectrum/button/package.json # packages/@react-spectrum/calendar/package.json # packages/@react-spectrum/checkbox/package.json # yarn.lock

rspbot · 2024-05-03T01:21:50Z

Build successful! 🎉

reidbarber

Nice!

rspbot · 2024-05-08T01:21:26Z

Build successful! 🎉

rspbot · 2024-05-08T01:21:28Z

## 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' }

snowystinger changed the base branch from main to fix-canary-test-useInteractOutside April 4, 2024 03:49

reidbarber mentioned this pull request Apr 4, 2024

WIP: Fix canary tests #6144

Closed

5 tasks

snowystinger changed the base branch from fix-canary-test-useInteractOutside to main April 9, 2024 04:27

snowystinger added 12 commits April 9, 2024 14:30

Storybook 7

acbd030

storybook frame renders

b0d063d

fix story sort

079b253

install other storybook packages

804d9b2

dedupe parcel versions

9807703

theoretically should work with 2.12 as macro support was released in it

6d08772

fix up addons

b7bf289

dedupe storybook installed packages

b962254

eliminating differences

d93c8e3

get storybook running

72e1db8

Configure canary prod build

da13e22

fix up chromatics and tests

fe93959

snowystinger force-pushed the sb-7 branch from e5ff522 to fe93959 Compare April 9, 2024 07:50

fix engines

d42bd40

snowystinger marked this pull request as ready for review April 9, 2024 11:23

fix TS

1777dd0

Trying to fix ?babel?

2bbf03b

ktabors reviewed Apr 10, 2024

View reviewed changes

Fix chromatic story

66112a7

snowystinger added 2 commits April 11, 2024 15:05

fix react 16 and 17 storybooks

daf68f5

Merge branch 'main' into sb-7

c6ed446

# Conflicts: # package.json # yarn.lock

reidbarber previously approved these changes Apr 17, 2024

View reviewed changes

snowystinger dismissed reidbarber’s stale review via 9d5f63f April 22, 2024 03:33

Merge branch 'main' into sb-7

ebe4a43

# Conflicts: # packages/@react-spectrum/card/test/Card.test.js # packages/@react-spectrum/menu/test/SubMenuTrigger.test.tsx

Merge branch 'main' into sb-7

3eba781

LFDanLu reviewed Apr 25, 2024

View reviewed changes

snowystinger added 3 commits May 2, 2024 14:04

fix tests

f9a0f9b

Merge branch 'main' into sb-7

1a2ba88

LFDanLu approved these changes May 3, 2024

View reviewed changes

reidbarber approved these changes May 3, 2024

View reviewed changes

Merge branch 'main' into sb-7

4be672d

snowystinger merged commit 8ed86d8 into main May 8, 2024
26 checks passed

snowystinger deleted the sb-7 branch May 8, 2024 01:44

devongovett mentioned this pull request Oct 2, 2024

Bump browserslist resolution to 4.24 #7137

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Storybook 7 #6150

Storybook 7 #6150

snowystinger commented Apr 4, 2024 •

edited

Loading

sookmax commented Apr 4, 2024

reidbarber commented Apr 4, 2024

sookmax commented Apr 4, 2024

reidbarber commented Apr 4, 2024

snowystinger commented Apr 4, 2024 •

edited

Loading

sookmax commented Apr 5, 2024

snowystinger commented Apr 9, 2024

sookmax commented Apr 9, 2024

LFDanLu commented Apr 9, 2024

rspbot commented Apr 10, 2024

ktabors left a comment

rspbot commented Apr 10, 2024

rspbot commented Apr 10, 2024

LFDanLu commented Apr 11, 2024 •

edited

Loading

snowystinger commented Apr 11, 2024 •

edited

Loading

rspbot commented Apr 12, 2024

rspbot commented Apr 22, 2024

rspbot commented Apr 23, 2024

rspbot commented Apr 24, 2024

LFDanLu left a comment

LFDanLu Apr 25, 2024

LFDanLu Apr 25, 2024

snowystinger Apr 25, 2024

LFDanLu May 3, 2024

rspbot commented May 3, 2024

reidbarber left a comment

rspbot commented May 8, 2024

rspbot commented May 8, 2024

Storybook 7 #6150

Storybook 7 #6150

Conversation

snowystinger commented Apr 4, 2024 • edited Loading

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

sookmax commented Apr 4, 2024

reidbarber commented Apr 4, 2024

sookmax commented Apr 4, 2024

reidbarber commented Apr 4, 2024

snowystinger commented Apr 4, 2024 • edited Loading

sookmax commented Apr 5, 2024

snowystinger commented Apr 9, 2024

sookmax commented Apr 9, 2024

LFDanLu commented Apr 9, 2024

rspbot commented Apr 10, 2024

ktabors left a comment

Choose a reason for hiding this comment

rspbot commented Apr 10, 2024

rspbot commented Apr 10, 2024

LFDanLu commented Apr 11, 2024 • edited Loading

snowystinger commented Apr 11, 2024 • edited Loading

rspbot commented Apr 12, 2024

rspbot commented Apr 22, 2024

rspbot commented Apr 23, 2024

rspbot commented Apr 24, 2024

LFDanLu left a comment

Choose a reason for hiding this comment

LFDanLu Apr 25, 2024

Choose a reason for hiding this comment

LFDanLu Apr 25, 2024

Choose a reason for hiding this comment

snowystinger Apr 25, 2024

Choose a reason for hiding this comment

LFDanLu May 3, 2024

Choose a reason for hiding this comment

rspbot commented May 3, 2024

reidbarber left a comment

Choose a reason for hiding this comment

rspbot commented May 8, 2024

rspbot commented May 8, 2024

snowystinger commented Apr 4, 2024 •

edited

Loading

snowystinger commented Apr 4, 2024 •

edited

Loading

LFDanLu commented Apr 11, 2024 •

edited

Loading

snowystinger commented Apr 11, 2024 •

edited

Loading