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

Upgrade to Storybook v8 and latest Solid #10

Merged
merged 32 commits into from
Nov 2, 2024

Conversation

JonahPlusPlus
Copy link
Collaborator

@JonahPlusPlus JonahPlusPlus commented Oct 12, 2024

This project is long overdue for an update, so I'm giving it a shot.

The main goal of this update is to get it on Storybook v8 and the latest version of SolidJS.

Work done so far:

  • Removed old artifacts (like Lerna git heads)
  • Updated yarn
  • Made bundling experience closer to main repo
  • Fixed out-of-date code
  • Updated dependencies
  • Added .editorconfig

Things to do:

  • Make sure reactivity is preserved (when splitting the render.tsx into renderToCanvas.tsx)
  • Investigate the use of delay (there appears to be a 20ms delay to take care of a race condition in async code; smells really bad)
  • Investigate use of npx to run jiti on other platforms (jiti doesn't appear on Windows, so I call it through npx)
  • Make sure exports are set up correctly
  • Update templates
  • Add in documentation for future devs
  • Update README
  • Investigate Vite's deprecation of the CJS build of the Node API
  • Investigate race condition in bundler
  • Add CI for formatting and linting
  • Refactor CD actions
  • Add bundling check script

Right now, my branch appears to work (with a small project linked via yarn). I say appears to work, since I never used Storybook (came from React Cosmos and my own Vite tool), so I have a limited frame of reference to use.

- Removed old artifacts (like Lerna githeads)
- Updated yarn
- Made bundling experience closer to main repo
- Fixed out-of-date code
- Updated dependencies
- Added .editorconfig
@JonahPlusPlus
Copy link
Collaborator Author

JonahPlusPlus commented Oct 13, 2024

@webblocksapp Hey, is there a reason for delaying rendering in docs mode? It seems to work properly without it (at least in Storybook v8).

@webblocksapp
Copy link
Collaborator

Hello @JonahPlusPlus , I’m currently not very active in contributing to this library. I added this delay because the Storybook documentation mode took a while to render, and sometimes the screen would remain blank. If you find that this delay is unnecessary and everything works fine without it, feel free to discuss the PR with the Storybook team through their official Discord channel. Best regards.

@shilman
Copy link
Member

shilman commented Oct 13, 2024

@JonahPlusPlus Exciting, thanks so much for picking this up! If you have any Storybook q's please give us a holler here or in our Discord and we'll do our best to help

@JonahPlusPlus
Copy link
Collaborator Author

@webblocksapp Understood, thanks for the super quick reply! I suppose I'll try creating more stories until I get some lag and see if the problem remains.

@shilman No problem, I need something like this in my projects and I would rather not write an entire library from scratch!

@shilman
Copy link
Member

shilman commented Oct 13, 2024

@JonahPlusPlus If you're coming from Cosmos and have a few minutes to spare, I'd love to ask you some questions about things we might do better in Storybook.

@JonahPlusPlus
Copy link
Collaborator Author

JonahPlusPlus commented Oct 13, 2024

@shilman Sure, we can discuss it on the Discord server (same handle).

@JonahPlusPlus
Copy link
Collaborator Author

Everything is almost done, but I ran into that blank canvas bug when running hundreds of stories via MDX in docs mode. I'm going to have to figure out how to wait for Solid to re-render before returning (if that is the case).

@JonahPlusPlus
Copy link
Collaborator Author

I improved the workaround for the race condition. My working theory is that it's a scheduling issue, probably with Storybook and the browser, due to assumptions with how many other frameworks render. Instead of using a delay, it's enough to space out render calls by using a semaphore (I noticed that calling timeout with 0 delay was enough to get just one story to render, which means it's likely related to DOM rendering).

Now I tested with rendering ~700 stories with no issue (200 was enough to get the bug). I also noticed the bug showed up in both docs and normal mode, so this change applies to both.

@JonahPlusPlus
Copy link
Collaborator Author

Refactored the CD scripts to use yarn idiomatically.

@JonahPlusPlus JonahPlusPlus marked this pull request as ready for review October 24, 2024 15:28
@JonahPlusPlus
Copy link
Collaborator Author

Ready for review! That took longer than expected (I was really busy with midterms and school projects), but it's finally at a point where I think it's acceptable.

Future work:

  • Investigate more robust solutions to the race condition (either in Storybook or Solid).
  • Add features listed in README

@shilman
Copy link
Member

shilman commented Oct 26, 2024

@JonahPlusPlus Thanks so much for reviving this!!!! This is an impressive set of changes 🙌

Can we please add:

  • A script for building the repo
  • A "hello world" Solid project in the monorepo with a storybook, for testing purposes

This might be a good reference project -- another framework developed outside of the monorepo that has a test app & a test lib: https://github.com/literalpie/storybook-framework-qwik

@TheComputerM
Copy link

Also instead on using common js, it would be better to use esm in preset.js in packages/frameworks/solid-vite since:

  1. Node has good support for ESM so running it shouldn't be a problem
  2. package.json has "type": "module"
  3. Storybook crashes if using require on node v23.1.0 with ReferenceError: module is not defined

README.md Outdated Show resolved Hide resolved
JonahPlusPlus and others added 4 commits October 30, 2024 15:20
Co-authored-by: Michael Shilman <shilman@users.noreply.github.com>
@JonahPlusPlus
Copy link
Collaborator Author

@shilman okay, I completed the requested changes (wish I could have done it faster, but school projects kept getting in my way)

@TheComputerM I wasn't able to get it working. Perhaps there was a fault in my testing (I used a project that worked with the CJS version, but it kept giving errors about not finding the module, so maybe I didn't properly name the files). I think I want to hold off on this for now, just so I can get this update out.

@JonahPlusPlus
Copy link
Collaborator Author

Actually, I suppose I can mess around more with Vite's ESM API.

@JonahPlusPlus
Copy link
Collaborator Author

Okay, the example now uses Vite's ESM API. I switched over to my laptop (I use Arch btw), and it became apparent that I just need to set the project type to "module" for the example. I don't see any need to use ESM for the presets (since the CJS works fine without any warning, but if there is some other reason that's undocumented, I would love to hear it).

Lesson learnt: don't use yarn on Windows. It breaks too often, which makes stuff like this confusing.

@shilman shilman added the enhancement New feature or request label Nov 2, 2024
@shilman shilman changed the title 1.0.0-beta.3 Update Upgrade to Storybook v8 and latest Solid Nov 2, 2024
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work @JonahPlusPlus !!! Thanks so much for taking this on & for your patience getting it reviewed/merged.

@shilman shilman merged commit 73abe6c into storybookjs:main Nov 2, 2024
3 checks passed
@Chudesnov
Copy link

Chudesnov commented Nov 5, 2024

TypeScript types seem to be broken in the new version released after this got merged. The only contents of index.d.ts are now:

// dev-mode
export * from '../src/index';

but there is no src/ directory in the published package (likely stripped out as a pre-publish step).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants