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

CLI: Fix Javascript language detection #23426

Merged
merged 1 commit into from
Jul 12, 2023

Conversation

valentinpalkovic
Copy link
Contributor

Closes #23390

What I did

Fixed language detection to circumvent detecting Typescript, although t isn't a direct dependency

How to test

  1. npx create-react-app my-app
  2. Initialize Storybook on top of it
  3. Javascript stories should be generated instead of Typescript ones.

-->

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • If this PR should be tested against many or all sandboxes,
    make sure to add the ci:merged or ci:daily GH label to it.
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

@valentinpalkovic valentinpalkovic self-assigned this Jul 12, 2023
@valentinpalkovic valentinpalkovic requested a review from yannbf July 12, 2023 13:39
@valentinpalkovic valentinpalkovic force-pushed the valentin/fix-typescript-version-check branch from 0df6fdd to 6ac7f17 Compare July 12, 2023 13:40
@valentinpalkovic valentinpalkovic force-pushed the valentin/fix-typescript-version-check branch from 6ac7f17 to d3f7139 Compare July 12, 2023 13:41
Copy link
Member

@yannbf yannbf left a comment

Choose a reason for hiding this comment

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

This change could cause the opposite effect on projects that are typescript but do not have typescript as a dependency/peerDependency, or where typescript comes from a "utility package" e.g. @my-company/developer-tools. I'm also not sure how this would behave in a monorepo that has a single package.json at the root.

Generally LGTM but I'd suggest trying this out in a monorepo setting

@valentinpalkovic valentinpalkovic merged commit fa6b477 into next Jul 12, 2023
@valentinpalkovic valentinpalkovic deleted the valentin/fix-typescript-version-check branch July 12, 2023 14:24
@github-actions github-actions bot mentioned this pull request Jul 12, 2023
15 tasks
@valentinpalkovic
Copy link
Contributor Author

@yannbf I think your concerns are valid, but the PR now more or less reverts the logic, which was introduced recently, and now the detection should behave the same way as in 7.0.x. I honestly do not know how to fix this issue in mono repo setups since the loaded package.json may vary. Should we possibly introduce a flag to make it possible to manually set the language for these cases?

@valentinpalkovic
Copy link
Contributor Author

We could also prompt a question if we find typescript somewhere in the node_modules, but not in the package.json, which is known to Storybook. This solution would be the cleanest.

@shilman shilman changed the title CLI: Fix language detection to properly detect Javascript CLI: Fix Javascript language detection Jul 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: npx storybook@next init installs the TypeScript template into a JavaScript project initialized by CRA
2 participants