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

SvelteKit: Add experimental page and navigation mocking #24795

Merged
merged 33 commits into from
Nov 22, 2023

Conversation

paoloricciuti
Copy link
Contributor

@paoloricciuti paoloricciuti commented Nov 9, 2023

Depends on #24921

Closes #20999

What I did

This PR adds a tighter integration for sveltekit allowing the user to mock the stores from sveltekit, adds an override listener for the links allowing you to execute code when a link it's clicked intead of navigating, adding the ability to still import and run code on afterNavigate (here emulated like onMount), adding callbacks for the other functions like goto, invalidateAll.

For the moment is a draft PR. I'm getting some weird compilation errors whenever i try to dynamically import @storybook/addon-actions but overall it should already work kinda fine.

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

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

  1. Run a sandbox for template sveltekit
  2. Modify the Button component to import { page } from "$app/stores"
  3. print the content of the store on the page with {JSON.stringify($page)}
  4. verify that the component normally work when sveltekit is launched
  5. Modify Button.stories.ts and add to the primary button parameters.sveltekit.stores.page={ test: true }
  6. Open Storybook in your browser
  7. Access the Button story
  8. verify that now the stringified page store contains { "test": true }

Documentation

  • Add or update documentation reflecting your changes

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>

@paoloricciuti paoloricciuti marked this pull request as ready for review November 10, 2023 16:13
@paoloricciuti
Copy link
Contributor Author

paoloricciuti commented Nov 10, 2023

I really had hopes for this last commit but apparently e2e tests are failing now. I've explored a bit figured out that the problem is that now trying to go to Example/Button (which is what e2e tests does) and clicking the button doesn't trigger the action (how it normally does and what e2e tests are looking for).

With a bit more exploration i figured out that it all boils down to this lines of code

export const previewAnnotations: StorybookConfig['previewAnnotations'] = (entry = []) => [
  ...entry,
  require.resolve('@storybook/sveltekit/preview'),
];

that i've added to the preset.ts file. If i remove those the action is triggered upon clicking the button (but the system introduced in this PR does not). If i add this back it's the opposite. My gut feeling is that adding this option override some global option that triggers the action. I've tried to add this to the preview.ts

export const parameters = {
	actions: { argTypesRegex: '^on[A-Z].*' },
},

without much success.

I'll try to explore a bit further.

EDIT:
Explored a bit more and i actually think this is some bug with the svelte renderer. Basically the issue is not in the previewAnnotation per se but in the fact that i'm using a Svelte component as a decorator. If i remove the svelte component as decorator everything works fine. I tried to make the component a single <slot /> which is just basically a passthrough component but still as soon as you add a svelte component decorator the action stop firing. The reason why i think this could be the case is that by wrapping the button component in another component storybook is not able to listen for the click event on the button itself but i might be wrong.

The optimal solution to this would actually be to ditch the Svelte component decorator and use a normal one but there's another bug, already tracked #24549 where normal decorator run twice in svelte.

@paoloricciuti
Copy link
Contributor Author

I was trying to fix one of the two bugs but i can't create a sandbox locally...i get this error which @JReinhold told me it's quite common...does anyone knows the fix for this?

➤ YN0071: │ Cannot link @storybook/codemod into @storybook/cli@portal:/Users/paoloricciuti/Desktop/code/storybook/code/lib/cli::locator=svelte-kit-skeleton-ts%40workspace%3A. dependency jscodeshift@npm:0.14.0 [7e949] conflicts with parent dependency jscodeshift@npm:0.15.1 [dfe9f]

@JReinhold JReinhold changed the title feat: tighter integration with sveltekit SvelteKit: Add page and navigation mocking Nov 20, 2023
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.

Super exciting!

I've partially reviewed this, all the way to code/frameworks/sveltekit/src/preview.ts

I'll review the rest ASAP.

code/frameworks/sveltekit/README.md Show resolved Hide resolved
code/frameworks/sveltekit/README.md Outdated Show resolved Hide resolved
code/frameworks/sveltekit/README.md Outdated Show resolved Hide resolved
code/frameworks/sveltekit/README.md Outdated Show resolved Hide resolved
code/frameworks/sveltekit/README.md Outdated Show resolved Hide resolved
code/frameworks/sveltekit/src/mocks/app/stores.ts Outdated Show resolved Hide resolved
code/frameworks/sveltekit/src/plugins/config-overrides.ts Outdated Show resolved Hide resolved
code/frameworks/sveltekit/src/preview.ts Show resolved Hide resolved
code/frameworks/sveltekit/src/preview.ts Outdated Show resolved Hide resolved
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.

Great work!!

code/frameworks/sveltekit/src/preview.ts Show resolved Hide resolved
code/frameworks/sveltekit/src/preview.ts Show resolved Hide resolved
code/frameworks/sveltekit/src/preview.ts Outdated Show resolved Hide resolved
code/frameworks/sveltekit/README.md Outdated Show resolved Hide resolved
@JReinhold
Copy link
Contributor

@paoloricciuti I did a bunch of stuff here and I think we're good now!

  • Pulled in Svelte: Fix decorators always running twice #24921 to ensure decorators were only called once, so tests would pass. We'll just wait with merging this until that is merged. (I wanted to telescope on top of it but realized that I couldn't because it's a fork)
  • Added a default link callback to log to the Actions panel (I don't know why addon-actions broke everything for you 🤷, it looks to be working fine)
  • Cleaned up the decorator a bit with types and a simpler code flow
  • Added e2e tests for default actions
  • fixed a small bug in Svelte: Fix decorators always running twice #24921 where the e2e test would fail in dev mode
  • made the test stories a bit more elaborate.

@JReinhold JReinhold added ci:daily Run the CI jobs that normally run in the daily job. and removed ci:normal labels Nov 22, 2023
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.

🎉

@JReinhold JReinhold changed the title SvelteKit: Add page and navigation mocking SvelteKit: Add experimental page and navigation mocking Nov 22, 2023
@JReinhold JReinhold merged commit e0509fe into storybookjs:next Nov 22, 2023
94 of 97 checks passed
@github-actions github-actions bot mentioned this pull request Nov 22, 2023
36 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:daily Run the CI jobs that normally run in the daily job. feature request sveltekit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: SvelteKit mocks
2 participants