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

Fix Storybook preview config being set up in old config directory #11869

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

Philzen
Copy link
Contributor

@Philzen Philzen commented Dec 29, 2024

@Tobbe this should definitely go into the next patch release as it is broken since v8.0.0.

Got a couple of more optimizations for chakra-ui setup in the pipeline that can build on this but are non-critical additions that can go into a minor or major version bump.

@Philzen
Copy link
Contributor Author

Philzen commented Dec 29, 2024

Also fixing (well, working around would be more accurate) this issue which would generate an invalid preview file for chakra-ui all along when running yarn rw setup ui chakra-ui:

grafik

The root cause is that this

const merged = await merge(storybookPreviewContent, newConfigContent, {
ImportDeclaration: interleave,
ArrayExpression: concatUnique,
ObjectExpression: concatUnique,
ArrowFunctionExpression: keepBothStatementParents,
FunctionDeclaration: keepBoth,
})

correctly merges the file, however it doesn't filter out if the file it merges with already has a default export.

Hence we need to ensure the i18n, mantine & chakra-ui setup templates do not contain a default export (which the first two don't) to work around this issue for the moment.

Philzen added a commit to Philzen/redwood that referenced this pull request Dec 29, 2024
@Philzen Philzen marked this pull request as ready for review December 29, 2024 18:35
@Tobbe
Copy link
Member

Tobbe commented Dec 29, 2024

Thanks @Philzen 🙏
Can you also please take a look at the failing test cases?

@Tobbe Tobbe self-assigned this Dec 29, 2024
@Tobbe Tobbe added the release:fix This PR is a fix label Dec 29, 2024
@Tobbe Tobbe added this to the next-release-patch milestone Dec 29, 2024
@Philzen
Copy link
Contributor Author

Philzen commented Dec 29, 2024

Can you also please take a look at the failing test cases?

Done.

With a few years of background in unit testing, looking at the concerned packages/project-config/src/__tests__/paths.test.ts IMHO:

  1. it shouldn't repeat the implementation (which it does 4 times 🙈) → this is the reason it failed, although it shouldn't because from a first glance it is testing a completely different aspect)
  2. it therefore could be ~1000 lines shorter

@Philzen Philzen force-pushed the bugfix/11864-storybook-templates-are-setup-in-old-config-directory branch from 0edd4da to bcad634 Compare December 30, 2024 02:06
@Philzen Philzen force-pushed the bugfix/11864-storybook-templates-are-setup-in-old-config-directory branch from bcad634 to 8b7a4d6 Compare December 30, 2024 02:35
@Philzen
Copy link
Contributor Author

Philzen commented Dec 30, 2024

  1. it shouldn't repeat the implementation…
  2. it therefore could be ~1000 lines shorter

OK i may have gone a little crazy here, but i thought i'd give it a try to increase the test maintainability and hence reduce a little technical debt.

Check cbacc5f...8b7a4d6

I could only save 480 lines of test code … which is better than nothing, but i probably promised a bit too much. As the implementation / single-source-of-truth is not exported in a consumable manner, i could only refactor the tests so they still repeat it … but now it at least only repeats it once except of four times and is much more readable IMHO. Thinking of #11760 where now we'd be able to delete four more lines out of this test and make everything more complete.

Technically this is way out-of-scope – so @Tobbe if you want i can pick them over into a separate PR for cleaner separation of concerns.

@Philzen Philzen changed the title Fix Storybook preview config is setup in old config directory Fix Storybook preview config being set up in old config directory Dec 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix This PR is a fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: setup ui & setup i18n commands still use old storybook folders
2 participants