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

Svelte: Improve CSF3 types #19512

Merged
merged 11 commits into from
Oct 23, 2022
Merged

Svelte: Improve CSF3 types #19512

merged 11 commits into from
Oct 23, 2022

Conversation

kasperpeulen
Copy link
Contributor

@kasperpeulen kasperpeulen commented Oct 17, 2022

What I did

This PR provides improved type safety for Svelte stories (similar to what we changed to React) but requires svelte-check instead of tsc (Svelte for VS Code extension for editor support):
https://github.com/sveltejs/language-tools/tree/master/packages/svelte-check

Requires the satisfies keyword available in TS ≥ 4.9:
microsoft/TypeScript#46827

Built on top of this CSF PR:
ComponentDriven/csf#51

Typesafe args

We changed StoryObj and Meta to increase type safety for when the user provides args partially in meta.

Considering a Component like this:

<script lang="ts">
  import { createEventDispatcher } from 'svelte';

  export let disabled: boolean;
  export let label: string;

  const dispatch = createEventDispatcher();
</script>

<button on:click={(e) => dispatch('clicked', e)} on:dblclick on:mousemove {disabled}>
  {label}
</button>

It is valid to provide args like this:

import Button from './Button.svelte';

// ✅ valid
const meta = {
  component: Button,
  args: { disabled: false },
} satisfies Meta<Button>;

export default meta;

type Story = StoryObj<typeof meta>;

export const Basic = {
  args: { label: 'good' }
} satisfies Story;

While it is invalid to forget an arg, in either meta or the story:

// ❌ invalid
const meta = {
  component: Button,
} satisfies Meta<Button>;

export const Basic = {
  args: { label: 'good' }
} satisfies Story;

// ❌ invalid
const meta = {
  component: Button,
  args: { label: 'good' }
} satisfies Meta<Button>;

export const Basic = {} satisfies Story;

Changed Meta to make sure both a Component, as the Props of the component can be used:

const meta = {
  component: Button,
} satisfies Meta<Button>;

export default meta;

export const meta = {
  component: Button,
} satisfies Meta<ComponentProps<Button>>;

export default meta;

If the component is used, you will have enhanced autocompletion for events (maybe in a future version also for slots, when we support that):

const meta = {
  component: Button,
  args: { label: 'good', disabled: false },
  render: (args) => ({
    Component: Button,
    props: args,
    on: {
      mousemove: (event) => {
        expectTypeOf(event).toEqualTypeOf<CustomEvent>();
      },
    },
  }),
} satisfies Meta<{ disabled: boolean; label: string }>;

Typesafe decorators/loaders

Decorators now accept a new generic arg argument to be specified:

const withDecorator: DecoratorFn<{ decoratorArg: string }> = (
  storyFn,
  { args: { decoratorArg } }
) => ({
  Component: Decorator,
  props: { decoratorArg },
});

And the type of meta/story will check if this arg is part of the generic type:

type Props = ComponentProps<Button> & { decoratorArg: string };

const meta = satisfies<Meta<Props>>()({
  component: Button,
  args: { disabled: false },
  decorators: [withDecorator],
});

type Story = StoryObj<typeof meta>;
const Basic: Story = { args: { decoratorArg: 0, label: 'good' } };

How to test

You can test it by running: yarn nx run @storybook/svelte:check

@benmccann @j3rem1e If you could have a look :)

@socket-security
Copy link

socket-security bot commented Oct 17, 2022

Socket Security Report

👍 No new dependency issues detected in pull request

Socket.dev scan summary
Issue Status
Did you mean? ✅ no new possible package typos
Install scripts ✅ no new install scripts
Telemetry ✅ no new telemetry
Troll package ✅ no new troll packages
Malware ✅ no new malware
Native code ✅ no new native modules
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

⚠️ Please accept the latest app permissions to ensure bot commands work properly. Accept the new permissions here.

Powered by socket.dev

@kasperpeulen kasperpeulen force-pushed the future/CSF3-svelte branch 2 times, most recently from 387630a to 44b95f0 Compare October 18, 2022 08:54
@kasperpeulen kasperpeulen marked this pull request as ready for review October 18, 2022 08:55
Copy link
Contributor

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

The PR description mentions TypeScript 4.9, but that's not released yet and it looks like this is using ~4.6.3?

It looks like there's tons of merge conflicts on this PR

scripts/utils/options.test.ts Outdated Show resolved Hide resolved
@kasperpeulen
Copy link
Contributor Author

The PR description mentions TypeScript 4.9, but that's not released yet and it looks like this is using ~4.6.3?

It looks like there's tons of merge conflicts on this PR

@benmccann Yes, I'm working on upgrading to TS 4.9 in this branch:
#19462

But that is blocked by TS 4.9 being released (and the ecosystem around it ESLint and prettier).
I will also update the svelte template in that branch later this week (for npx sb init).

This PR can be merged independently of that though, as satisfies is only used in user code, not in library code.

Resolving the merge conflicts 😅

# Conflicts:
#	code/addons/a11y/package.json
#	code/addons/actions/package.json
#	code/addons/backgrounds/package.json
#	code/addons/controls/package.json
#	code/addons/docs/package.json
#	code/addons/interactions/package.json
#	code/addons/links/package.json
#	code/addons/measure/package.json
#	code/addons/outline/package.json
#	code/addons/storyshots/storyshots-core/package.json
#	code/addons/storyshots/storyshots-puppeteer/package.json
#	code/examples/external-docs/package.json
#	code/frameworks/angular/package.json
#	code/lib/addons/package.json
#	code/lib/api/package.json
#	code/lib/blocks/package.json
#	code/lib/client-api/package.json
#	code/lib/codemod/package.json
#	code/lib/components/package.json
#	code/lib/core-client/package.json
#	code/lib/core-common/package.json
#	code/lib/core-server/package.json
#	code/lib/docs-tools/package.json
#	code/lib/preview-web/package.json
#	code/lib/store/package.json
#	code/renderers/html/package.json
#	code/renderers/preact/package.json
#	code/renderers/react/package.json
#	code/renderers/server/package.json
#	code/renderers/svelte/package.json
#	code/renderers/vue/package.json
#	code/renderers/vue3/package.json
#	code/renderers/web-components/package.json
#	code/ui/manager/package.json
#	code/yarn.lock
@yannbf yannbf changed the title Better CSF3 types for svelte Svelte: Improve CSF3 types Oct 19, 2022
# Conflicts:
#	code/lib/core-common/src/index.ts
# Conflicts:
#	code/addons/a11y/package.json
#	code/addons/actions/package.json
#	code/addons/backgrounds/package.json
#	code/addons/controls/package.json
#	code/addons/docs/package.json
#	code/addons/interactions/package.json
#	code/addons/links/package.json
#	code/addons/measure/package.json
#	code/addons/outline/package.json
#	code/addons/storyshots/storyshots-core/package.json
#	code/addons/storyshots/storyshots-puppeteer/package.json
#	code/examples/external-docs/package.json
#	code/frameworks/angular/package.json
#	code/lib/addons/package.json
#	code/lib/api/package.json
#	code/lib/blocks/package.json
#	code/lib/client-api/package.json
#	code/lib/codemod/package.json
#	code/lib/components/package.json
#	code/lib/core-client/package.json
#	code/lib/core-common/package.json
#	code/lib/core-server/package.json
#	code/lib/docs-tools/package.json
#	code/lib/preview-web/package.json
#	code/lib/store/package.json
#	code/renderers/html/package.json
#	code/renderers/preact/package.json
#	code/renderers/react/package.json
#	code/renderers/server/package.json
#	code/renderers/svelte/package.json
#	code/renderers/vue/package.json
#	code/renderers/vue3/package.json
#	code/renderers/web-components/package.json
#	code/ui/manager/package.json
#	code/yarn.lock
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.

This looks really good!

I'm still not sure what the impact is of using satisfies and TS 4.9 for our users, but I think you've discussed this earlier with people so I trust that it is under control.

code/package.json Outdated Show resolved Hide resolved
@@ -335,7 +336,7 @@
"ts-jest": "^26.4.4",
"ts-node": "^10.4.0",
"tsup": "^6.2.2",
"typescript": "4.7.4",
"typescript": "~4.6.3",
Copy link
Contributor

Choose a reason for hiding this comment

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

downgrade?

Copy link
Member

Choose a reason for hiding this comment

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

More likely ensuring we use the same version everywhere.

code/renderers/svelte/src/___test___/Button.svelte Outdated Show resolved Hide resolved
@@ -0,0 +1,230 @@
import { describe, test } from '@jest/globals';
Copy link
Contributor

Choose a reason for hiding this comment

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

I love these type tests. 💪

"strict": true,
"resolveJsonModule": true
},
"include": ["src/**/*"],
"exclude": ["src/**/*.test.*"]
"exclude": []
Copy link
Contributor

Choose a reason for hiding this comment

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

unless this has an effect, I'd remove this line.

Copy link
Member

Choose a reason for hiding this comment

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

I think It's likely required, because of:

"exclude": [
"dist",
"**/dist",
"node_modules",
"**/node_modules",
"**/*.spec.ts",
"**/__tests__",
"**/*.test.ts",
"**/setup-jest.ts"
],

code/package.json Outdated Show resolved Hide resolved
# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
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.

5 participants