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

React: Move argType stories to template/stories folder #19265

Merged
merged 23 commits into from
Sep 30, 2022

Conversation

tmeasday
Copy link
Member

What I did

  • Move the stories that test argType detection (in react) to the template/stories folder.
  • As this is a .tsx file, it'll only appear in TS-based react sandboxes.
  • Note I needed to comment out 4 test cases that required styled components or emotion. They still seem to pass unit tests though (for now).

How to test

  • We should check the chromatic stories match the old stories in official storybook before merging

@tmeasday tmeasday requested a review from shilman September 27, 2022 04:28
@tmeasday tmeasday added maintenance User-facing maintenance tasks react addon: docs labels Sep 27, 2022
@tmeasday tmeasday force-pushed the tom/sb-752-addon-docs-argtype-inference branch from 9c07009 to 9e4438a Compare September 27, 2022 04:31
@tmeasday
Copy link
Member Author

tmeasday commented Sep 27, 2022

This PR is currently breaking in vite, not sure why.

It seems as if docgen isn't working in the vite build for JS files, I'm not sure if this is expected @shilman?

@tmeasday
Copy link
Member Author

@shilman two things blocking me right now:

  1. Jest tests failing because we cannot use ESM code in these files due to the use of requireFromString (I think). However we need to write ESM for these stories to work in Vite. Any idea which we should prefer? (obvious things we could do: a. skip these examples in the jest tests; b. remove them from all sandboxes).

  2. Styled components TS test case causing build to fail (annoying because we aren't even including it in the SB). Should we just delete these stories? (Eventually we won't have SC in the monorepo anyway and that'll be an issue eventually).

@shilman
Copy link
Member

shilman commented Sep 28, 2022

@tmeasday let's delete the styled components code & bring it back later in the annotations server if we even need it at all

As for Jest vs Vite, I think we prefer Vite at this point no question

It was blowing up the CRA build even though it wasn't being referenced 😠
@shilman shilman changed the title Move argType stories to template/stories folder React: Move argType stories to template/stories folder Sep 29, 2022
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.

LGTM. Thanks for toughing this one out!

@tmeasday
Copy link
Member Author

tmeasday commented Sep 29, 2022

The issue here seems to be #17458 -- basically that assert is not available in the browser environment. This seems to be a problem with doctrine that isn't ever getting fixed. @IanVS @joshwooding what do you think about adding an alias for assert to the vite builder, similar to https://medium.com/@ftaioli/using-node-js-builtin-modules-with-vite-6194737c2cd2 (but probably using browser-assert like we do in the manager and webpack builder)??

@joshwooding
Copy link
Member

The issue here seems to be #17458 -- basically that assert is not available in the browser environment. This seems to be a problem with doctrine that isn't ever getting fixed. @IanVS @joshwooding what do you think about adding an alias for assert to the vite builder, similar to https://medium.com/@ftaioli/using-node-js-builtin-modules-with-vite-6194737c2cd2 (but probably using browser-assert like we do in the manager and webpack builder)??

This has come up previously, I think ultimately we would want to move away from doctrine since it’s no longer being supported but for now adding a shim for assert seems like a good mitigation for now. 👍

@tmeasday tmeasday merged commit d640195 into next Sep 30, 2022
@tmeasday tmeasday deleted the tom/sb-752-addon-docs-argtype-inference branch September 30, 2022 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addon: docs maintenance User-facing maintenance tasks react
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants