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

Docs: Show source of corresponding args in source block #20915

Merged
merged 15 commits into from
Feb 14, 2023

Conversation

tmeasday
Copy link
Member

@tmeasday tmeasday commented Feb 3, 2023

Closes #20745

What I did

  • Emit args with SNIPPET_RENDERED event
  • Store code in source container keyed on args hash
  • Read code based on args except if __forceInitialArgs is passed to Source (in which case read on initialArgs).
  • Pass __forceInitialArgs through.

How to test

See stories & E2E tests. Try changing args on docs page with source open and check only the primary story changes.

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"]

@socket-security
Copy link

socket-security bot commented Feb 3, 2023

Socket Security Pull Request Report

👍 No new dependency issues detected in pull request

Pull request report summary
Issue Status
Install scripts ✅ 0 issues
Native code ✅ 0 issues
Bin script confusion ✅ 0 issues
Bin script shell injection ✅ 0 issues
Shell access ✅ 0 issues
Uses eval ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
GitHub dependency ✅ 0 issues
New author ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues
AI detected malware ✅ 0 issues
Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@2.4.2

Powered by socket.dev

Copy link
Contributor

@JReinhold JReinhold left a comment

Choose a reason for hiding this comment

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

Code looks good.

Still concerned about this though #20745 (comment)

This would require all renderers to change right, even the community ones we don't control?

code/renderers/react/src/docs/jsxDecorator.tsx Outdated Show resolved Hide resolved
@tmeasday tmeasday marked this pull request as ready for review February 6, 2023 04:11
@tmeasday tmeasday requested a review from JReinhold February 6, 2023 04:11
@tmeasday tmeasday added the ci:daily Run the CI jobs that normally run in the daily job. label Feb 6, 2023
@tmeasday
Copy link
Member Author

tmeasday commented Feb 6, 2023

@JReinhold note the fallback to "unknown args" when we don't receive args details on the channel message.

Copy link
Contributor

@JReinhold JReinhold left a comment

Choose a reason for hiding this comment

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

LGTM! Only a minor nit.

code/ui/blocks/src/blocks/SourceContainer.tsx Outdated Show resolved Hide resolved
@tmeasday
Copy link
Member Author

tmeasday commented Feb 8, 2023

@shilman as discussed, the E2E test I added here which inspects the arg value in the snippet is failing in various frameworks that don't appear to be rendering the snippet (should contain something like label="Basic") correctly:

Angular:

<storybook-framework-button
  label=\"false\"
  (onClick)=\"onClick($event)\"
  ></storybook-framework-button>

Lit:

<sb-button></sb-button>

Next: - known issue: #20356

<head_manager_provider_default>
  <No Display Name />
</head_manager_provider_default>"

In Vue it renders the right thing but never updates based on args changes. It looks like the SNIPPET_RENDERED event isn't emitted on args changes.

@tmeasday tmeasday merged commit d863378 into next Feb 14, 2023
@tmeasday tmeasday deleted the 20745-duplicate-source-snippets branch February 14, 2023 05:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addon: docs bug ci:daily Run the CI jobs that normally run in the daily job.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Source snippets clobber each other when you render a story more than once
3 participants