Svelte: Migrate @storybook/svelte to Typescript #8770

aromanarguello · 2019-11-09T01:31:47Z

vercel · 2019-11-09T01:31:53Z

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/storybook/monorepo/f132e08ce
🌍 Preview: https://monorepo-git-fork-aromanarguello-sveltetypescriptmigration.storybook.now.sh

shilman

Great job @aromanarguello!!! A few minor comments above

app/svelte/package.json

shilman · 2019-11-09T07:21:44Z

app/svelte/package.json

    "@storybook/core": "5.3.0-alpha.41",
    "core-js": "^3.0.1",
    "global": "^4.3.2",
    "regenerator-runtime": "^0.13.3",
-    "ts-dedent": "^1.1.0"
+    "ts-dedent": "^1.1.0",
+    "webpack": "^4.33.0"


I added webpack to be able to import the Configuration type

import { Configuration } from 'webpack'; export function webpack(config: Configuration) {

in the framework-preset-svelte. What would be the best alternative to this? @shilman

Does webpack now ships its own types? There's also https://www.npmjs.com/package/@types/webpack

In app/angular @ndelangen added it to devDeps and peerDeps along with @types/webpack-env 🤔 (according to git blame)
https://github.com/storybookjs/storybook/blame/next/app/angular/package.json

In app/react webpack it is a direct dependency and also includes @types/webpack as a devDep

in app/vue it's the same as in app/react

Back to this PR: would it work with only @types/webpack as devDependency?

Shipping new direct dependencies is sometimes the root cause of new issues. Do svelte apps install it's own webpack dependency? If yes, then we should ship it as a peerDependency. If not then something under the hood of svelte might already ship it as a direct dependency (kind of like angular)

If svelte does not ship webpack at all it might be ok to ship a directDependency of webpack

@gaetanmaisse Can you assist on this as well?

I think there is no need to add webpack as dependency neither as devDependency. What leads me to that is that:

Configuration type is used in server part of @storybook/svelte which is not exported to the outside world via types/main properties of package.json, only client is.

This package does not use webpack directly, it just override/create a webpack config.

This package has @storybook/core as a dependency, the latter uses webpack and have it as dependencies

However, just removing webpack from package.json leads to a no-extraneous-dependencies eslint error in server/framework-preset-svelte.ts. But maybe our context is a good reason to disable this rule for this import?

We should maybe add @types/webpack as devDependency as we are using Configuration type in svelte/src/server codebase 🤷‍♂ even if everything looks good to TS compiler without adding it (surely because @types/webpack is required by another module of SB monorepo). Does anyone know TS best practices when we want to use some dependency typings but not the dependency itself?

What's your mind about it?

I'm fine with ignoring the no-extraneous-dependencies warning

Rethinking about it, this issue will maybe be solved when @storybook/core will be migrated to TS.

If @storybook/core defines and exports a WebpackConfiguration type:

import { Configuration } from 'webpack'; export type WebpackConfiguration = Configuration

then using it in @storybook/svelte will not cause no-extraneous-dependencies as @storybook/core is a dependency. And everything should be good for TS if @storybook/core will have webpack and @types/webpack as dependencies.

How do we want to proceed in the meantime? Should we mark config:any until @storybook/core is migrated, or ignoring the no-extraneous-dependencies warning?

@aromanarguello You can add the comment to ignore no-extraneous-dependencies rule for this line, remove webpack from dependencies and 🚀

app/svelte/src/client/preview/render.ts

aromanarguello · 2019-11-09T19:30:04Z

All comments addressed :) LMK if any other changes are necessary @shilman

gaetanmaisse · 2019-11-10T18:41:46Z

app/svelte/src/index.d.ts

@@ -0,0 +1,2 @@
+declare module '@storybook/core/*';


This file should be named app/svelte/src/typings.d.ts instead of app/svelte/src/index.d.ts

gaetanmaisse · 2019-11-10T19:23:39Z

app/svelte/tsconfig.json

+{
+  "extends": "../../tsconfig.json",
+  "compilerOptions": {
+    "rootDir": ".",


Use should set :rootDir": "./src", otherwise dist folder generated during TS "compilation" is not as expected:

In order to be able to do that you have to use const packageJson = require('../../package.json'); in src/server/options.ts instead of import packageJson from '../../package.json';. As it is done for react for instance:

storybook/app/react/src/server/options.ts

Line 1 in 9fb7217

const packageJson = require('../../package.json');

BTW, not related at all to your PR, there is a weird thing with *.d.ts files, they are compiled to JS files while they shouldn't. I will take a look when I have time to. cc @ndelangen @kroeder

Got it! Thanks for your advice and tips, I am learning a lot :)

gaetanmaisse · 2019-11-10T20:00:39Z

@aromanarguello Thanks a lot for your work 👏 ! I just made 2 TS related comments that you can easily fix 💪 . Your PR also puts in the light the webpack dep "issue", which is more global than just your PR 😉

shilman · 2019-11-11T02:47:42Z

app/svelte/src/server/options.ts

@@ -1,4 +1,4 @@
-import packageJson from '../../package.json';
+const packageJson = require('../../package.json');


why?? @gaetanmaisse

Just saw your comment. Still this seems weird to me. Any references I can look at?

By setting "rootDir": "./src" we are telling TS compiler that all sources will be in src folder. As package.json is not in src it can not be imported directly (i.e. at compile time) without throwing: File '/Path/to/package.json' is not under 'rootDir' '/Path/To/App/src/'. 'rootDir' is expected to contain all source files.

require will import the file at runtime so TS compiler will not try to load it and finally everyone is happy.

shilman · 2019-11-12T09:04:50Z

@gaetanmaisse @aromanarguello are we good to merge this now?

gaetanmaisse · 2019-11-12T13:11:03Z

@shilman, @aromanarguello will add one last commit for #8770 (comment) and we will be good

aromanarguello · 2019-11-12T13:53:30Z

all done @shilman @gaetanmaisse :)

shilman · 2019-11-12T14:05:35Z

@aromanarguello Congratulations on your first OSS PR merge!!! 🎉🎉🎉

@gaetanmaisse Thanks for shepherding!!! 🙏🙏🙏

vercel bot temporarily deployed to staging November 9, 2019 01:31 Inactive

aromanarguello force-pushed the svelteTypescriptMigration branch from 769ce18 to 5c91031 Compare November 9, 2019 01:34

vercel bot temporarily deployed to staging November 9, 2019 01:34 Inactive

vercel bot deployed to staging November 9, 2019 01:37 View deployment

shilman requested changes Nov 9, 2019

View reviewed changes

shilman added maintenance User-facing maintenance tasks typescript labels Nov 9, 2019

shilman added this to the 5.3.0 milestone Nov 9, 2019

shilman changed the title ~~Migrate @storybook/svelte to Typescript~~ Svelte: Migrate @storybook/svelte to Typescript Nov 9, 2019

shilman added the svelte label Nov 9, 2019

aromanarguello force-pushed the svelteTypescriptMigration branch from 379777c to bed95e7 Compare November 9, 2019 16:40

vercel bot temporarily deployed to staging November 9, 2019 16:40 Inactive

aromanarguello force-pushed the svelteTypescriptMigration branch from bed95e7 to e67b92e Compare November 9, 2019 16:42

vercel bot temporarily deployed to staging November 9, 2019 16:42 Inactive

vercel bot temporarily deployed to staging November 9, 2019 16:43 Inactive

aromanarguello force-pushed the svelteTypescriptMigration branch from c1ea4b1 to 35a6011 Compare November 9, 2019 16:52

vercel bot temporarily deployed to staging November 9, 2019 16:52 Inactive

vercel bot deployed to staging November 9, 2019 16:54 View deployment

aromanarguello force-pushed the svelteTypescriptMigration branch from b735fbb to 7d04427 Compare November 9, 2019 18:13

vercel bot temporarily deployed to staging November 9, 2019 18:13 Inactive

vercel bot deployed to staging November 9, 2019 18:13 View deployment

vercel bot deployed to staging November 10, 2019 02:58 View deployment

shilman approved these changes Nov 10, 2019

View reviewed changes

shilman requested review from ndelangen, gaetanmaisse, kroeder and rixo November 10, 2019 03:00

shilman added the help wanted label Nov 10, 2019

vercel bot deployed to staging November 10, 2019 03:36 View deployment

gaetanmaisse requested changes Nov 10, 2019

View reviewed changes

vercel bot deployed to staging November 11, 2019 02:23 View deployment

shilman reviewed Nov 11, 2019

View reviewed changes

kroeder mentioned this pull request Nov 11, 2019

TypeScript migration organization #5030

Closed

vercel bot deployed to staging November 11, 2019 22:39 View deployment

aromanarguello force-pushed the svelteTypescriptMigration branch from 44d5446 to 16bcc0e Compare November 11, 2019 23:42

vercel bot deployed to staging November 11, 2019 23:42 View deployment

vercel bot requested a deployment to staging November 12, 2019 00:08 Pending

vercel bot deployed to staging November 12, 2019 00:26 View deployment

aromanarguello and others added 7 commits November 12, 2019 08:13

Migrate @storybook/svelte to Typescript

e7663a5

fix types and addon version

243a732

fix lock file

d1ea47b

Update package.json

796fef1

update yarn file

860ff33

changed index to typings, fixed import in options.ts

3d46f7b

updated @storybook addon/core deps

945bc50

vercel bot temporarily deployed to staging November 12, 2019 13:19 Inactive

update lockfile

3c76269

aromanarguello force-pushed the svelteTypescriptMigration branch from aee7968 to 3c76269 Compare November 12, 2019 13:29

vercel bot deployed to staging November 12, 2019 13:29 View deployment

shilman merged commit 4f59937 into storybookjs:next Nov 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Svelte: Migrate @storybook/svelte to Typescript #8770

Svelte: Migrate @storybook/svelte to Typescript #8770

aromanarguello commented Nov 9, 2019

vercel bot commented Nov 9, 2019 •

edited

Loading

shilman left a comment

shilman Nov 9, 2019

aromanarguello Nov 9, 2019

kroeder Nov 10, 2019 •

edited

Loading

gaetanmaisse Nov 10, 2019 •

edited

Loading

ndelangen Nov 11, 2019

gaetanmaisse Nov 11, 2019

aromanarguello Nov 11, 2019 •

edited

Loading

gaetanmaisse Nov 12, 2019

aromanarguello commented Nov 9, 2019

gaetanmaisse Nov 10, 2019

gaetanmaisse Nov 10, 2019

gaetanmaisse Nov 10, 2019

aromanarguello Nov 11, 2019

gaetanmaisse commented Nov 10, 2019

shilman Nov 11, 2019

shilman Nov 11, 2019

gaetanmaisse Nov 11, 2019

shilman commented Nov 12, 2019

gaetanmaisse commented Nov 12, 2019

aromanarguello commented Nov 12, 2019

shilman commented Nov 12, 2019 •

edited

Loading

		@@ -1,4 +1,4 @@
		import packageJson from '../../package.json';
		const packageJson = require('../../package.json');

Svelte: Migrate @storybook/svelte to Typescript #8770

Svelte: Migrate @storybook/svelte to Typescript #8770

Conversation

aromanarguello commented Nov 9, 2019

vercel bot commented Nov 9, 2019 • edited Loading

shilman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kroeder Nov 10, 2019 • edited Loading

Choose a reason for hiding this comment

gaetanmaisse Nov 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aromanarguello Nov 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aromanarguello commented Nov 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaetanmaisse commented Nov 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shilman commented Nov 12, 2019

gaetanmaisse commented Nov 12, 2019

aromanarguello commented Nov 12, 2019

shilman commented Nov 12, 2019 • edited Loading

vercel bot commented Nov 9, 2019 •

edited

Loading

kroeder Nov 10, 2019 •

edited

Loading

gaetanmaisse Nov 10, 2019 •

edited

Loading

aromanarguello Nov 11, 2019 •

edited

Loading

shilman commented Nov 12, 2019 •

edited

Loading