Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Storybook/Export to CodeSandbox #19583
Storybook/Export to CodeSandbox #19583
Changes from 62 commits
43b340d
61ba427
442f248
0efe6ab
5ec023d
3da0b4f
c9d5c7d
5d74494
8034742
7a27b0b
1394163
074908e
685ea4d
2536d86
7eb2523
080702e
97c00aa
dd8ed8c
a6e25a9
d0c3fbb
b3b1c50
cedc49c
0e5187f
ddc13de
f05c67f
b20e6d5
d209ba6
7e52408
494981a
3d7a839
f3d5ec2
754d075
e982e55
e820ea0
3649556
d34b67d
a5d7507
e9af531
cb8fac9
375a1a7
cb323af
d178b4c
9b91353
6a36221
f05fc75
18686f6
4397531
b7d863e
1a8d42e
54a95b8
7d2536a
66b00f1
4ed4048
e422879
9c4a533
2ea8ee0
bec2094
f4b5fc5
ddc0d31
bdf0c54
3874ce0
eccd640
c5182fc
16d079a
9f048b9
8bad95c
f5644ab
d7572e0
07e461a
a1e0402
439617d
80a0b6c
1d8c1ae
cb3da40
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
This file was deleted.
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will break any type checking related to the component, which is probably not a good idea for maintainability or our internal dev experience. What was the reason you had to it this way? Then maybe we can come up with an alternative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a general principle, we should optimize for the primary scenario (local dev experience and ensuring types stay in sync), and if that doesn't work with some secondary scenario (export to codesandbox), any workarounds/hacks should be done in the secondary scenario's code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main reason for this is that we build storybook code along with other code. During the
build
process, we no longer use ts path aliases, which means that without this ts-ignore, the build will always fail. We want to use aliases here because we want to avoid circular dependencies between packages.As long as we use ts path aliases, the only long term solution for this is to use separate ts configs which is proposed in #19044
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to make the codesandbox export addon convert relative imports within the same package to absolute imports? That would at least get rid of some of the ts-ignores.
Especially if we end up renaming slots to be named with a
Slot
suffix, having type checking disabled all over the place is a huge risk.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only easy solution I could think of is specifying the dependency package and it’s version manually, on the import line:
Then, in export to CodeSandbox, we’d replace it with regex:
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way the addon could access package.json to read the package name and version? Or could they be read and passed in when configuring the addon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that would require more time than I can commit to this. I can create a GitHub issue about it if you think it’s worth it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's definitely worth doing something to avoid using a sledgehammer that so significantly detracts from maintainability. I did something sort of similar with
packages/react-examples/.storybook/preview-loader.js
, which is solving a different problem but the approach might be generally applicable. I'll check out your branch and see if I can come up with anything.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ecraig12345 I’ll merge this now without changing Button stories. Let’s address automatic version detection / absolute imports in other PRs.