Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build: Fix sandbox running multiple versions of react #19156

Merged
merged 5 commits into from
Sep 14, 2022

Conversation

shilman
Copy link
Member

@shilman shilman commented Sep 9, 2022

Issue: N/A

Template stories were running multiple versions of react in react projects when referenced by relative path.

@tmeasday @IanVS any idea why this breaks in vite?

What I did

  • Fixed this by sym-linking the stories into the project
  • Light refactor of sandbox script
  • Added react hooks stories to test this case

How to test

  • CI passes

@shilman shilman added the maintenance User-facing maintenance tasks label Sep 9, 2022
@shilman shilman requested review from tmeasday and IanVS September 9, 2022 17:04
@shilman
Copy link
Member Author

shilman commented Sep 10, 2022

@tmeasday Another problem that I'm seeing with this scheme is that the file watching for the story index server breaks. changes to the linked stories don't trigger an index update.

@IanVS
Copy link
Member

IanVS commented Sep 11, 2022

@shilman do you have an example of a failure that was happening with multiple react versions?

@shilman
Copy link
Member Author

shilman commented Sep 11, 2022

@IanVS If you copy the React hooks story from this PR into next and run a sandbox you'll see the problem when you navigate to that story. @tmeasday and I debugged this together and verified the solution. Symlinking the stories into the sandbox solves that problem, but didn't anticipate the follow-on problems that it reveals.

yarn sandbox --template cra/default-js

@tmeasday
Copy link
Member

@shilman do we need to install @storybook/jest as a dependency of our sandboxes?

@tmeasday
Copy link
Member

@tmeasday @IanVS any idea why this breaks in vite?

@shilman in what way is it breaking in vite (but not CRA)?

@tmeasday
Copy link
Member

Another problem that I'm seeing with this scheme is that the file watching for the story index server breaks. changes to the linked stories don't trigger an index update.

Hmm, we definitely need to investigate this, that would be very annoying.

@shilman
Copy link
Member Author

shilman commented Sep 11, 2022

@tmeasday See the CI failure

@tmeasday
Copy link
Member

tmeasday commented Sep 11, 2022

@shilman build-sandboxes? It is failing in all frameworks, not just Vite, for the @storybook/jest reason mentioned above:

image

smoke-test-sandboxes is only failing in CRA, but it isn't giving any useful output. Hopefully the same thing?

@shilman
Copy link
Member Author

shilman commented Sep 11, 2022

@tmeasday I'm not at my computer but I think there's also a dev failure that's only in vite. I'll report back later.

@shilman
Copy link
Member Author

shilman commented Sep 11, 2022

@tmeasday @IanVS my bad, i misread the CI output. it was just the missing dependency & i fixed by adding it back. LMK if there's a cleaner way to do this.

@shilman
Copy link
Member Author

shilman commented Sep 11, 2022

@tmeasday holding off on this until we figure out the story index issue for linked stories

@tmeasday
Copy link
Member

tmeasday commented Sep 12, 2022

@shilman

@tmeasday holding off on this until we figure out the story index issue for linked stories

I took a look at this. As far as I can tell reading the source, watchpack doesn't resolve internal symlinks when watching a directory.

A solution is just to add each symlinked dir as a stories glob:

image

This appears to work, although we'll have to go back to a more complex stories field unfortunately.

OTOH, changing it to a single glob kind of broke your original idea of being able to easily comment out different stories @shilman

@shilman
Copy link
Member Author

shilman commented Sep 12, 2022

NOTE: We should replace ../**/*.stories.* with ../(!template-stories)**/*.stories.* to avoid duplicate matches when there is no src dir in the sandbox.

@shilman shilman assigned shilman and unassigned ndelangen Sep 13, 2022
@shilman shilman merged commit 05664e4 into next Sep 14, 2022
@shilman shilman deleted the shilman/fix-sandbox-multiple-reacts branch September 14, 2022 06:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants