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

Improve mdx to csf codemod #24637

Open
wants to merge 4 commits into
base: next
Choose a base branch
from

Conversation

bodograumann
Copy link
Contributor

@bodograumann bodograumann commented Oct 31, 2023

What I did

This is my attempt to solve the issues I am having with the mdx-to-csf migration.
I realize that these code transformations might be too opinionated for the general audience.
So please tell me which ones are fine, and which ones should be refined, removed or somehow made optional.

  • Do not generate the story name attribute if it is redundant with the story identifier.
    Otherwise eslint-plugin-storybook complains.
  • Story names can conflict with the component name, leading to invalid js.
    I have widened the existing logic to deduplicate identifiers, that appends an underscore.
    TODO: I have to add the case of default exports, because that is how vue components are usually imported.
  • Lots of leftover unused imports and definitions. eslint can't remove them automatically.
    Here I am removing all exports from the .mdx result files.
    Earlier I also removed all imports which are not for doc blocks, but I encountered an instance in my own project where the component was imported to be used with ArgsTable.
    So for the imports I opted for the unused-imports eslint-plugin instead.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Are the snapshots in code/lib/codemod/src/transform/__testfixtures__/mdx-to-csf used anywhere? How can I check them locally?

Manual testing

I have tested the code mod with a local project and it works as expected.

Documentation

Is there any documentation regarding the codemod that should be updated?

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts

  • Make sure this PR contains one of the labels below:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>

@bodograumann bodograumann force-pushed the improve-mdx-to-csf-codemod branch 2 times, most recently from 7921c82 to 0a8ffd5 Compare October 31, 2023 20:27
@shilman
Copy link
Member

shilman commented Nov 6, 2023

Awesome work @bodograumann !!! We are taking a look this week 👀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Empathy Backlog
Development

Successfully merging this pull request may close these issues.

4 participants