-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Using Babel for TypeScript instead of tsc #5109
Changes from 55 commits
2fc1518
1a5fa4b
8543bf0
8c8775c
a40589a
c5f4775
468477d
2da70f6
d351d87
4be2657
b79e39a
65b8c31
36c738e
66b3cdb
22b919a
1e0abdd
8616481
44e566f
d551fb3
10a530d
45dcad3
14f8cbc
c1d2ba6
f0c4cb4
161fc1f
7ed0898
2f4f5b5
3f12a74
2162772
0a9626d
8a2919c
1194c5f
29db6ad
d667a92
37576e5
87dfbb7
6bccd3d
aa9fa9e
f19c1eb
33d9835
f3b83a1
4da6504
bfce809
4050dbc
4729fc4
7104e93
6e6df7c
10ac24b
45f0445
24e7eb4
2521d2b
413438a
ef57974
7237cd7
0b881e2
dc521ca
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,22 @@ | ||
// tslint:disable-next-line:no-implicit-dependencies | ||
import { IStory } from '@storybook/angular'; | ||
export interface ICollection { | ||
[p: string]: any; | ||
} | ||
|
||
export interface NgModuleMetadata { | ||
declarations?: any[]; | ||
entryComponents?: any[]; | ||
imports?: any[]; | ||
schemas?: any[]; | ||
providers?: any[]; | ||
} | ||
|
||
export interface IStory { | ||
props?: ICollection; | ||
moduleMetadata?: Partial<NgModuleMetadata>; | ||
component?: any; | ||
template?: string; | ||
} | ||
declare module '@storybook/addon-centered/angular' { | ||
export function centered(story: IStory): IStory; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
const path = require('path'); | ||
import { resolve } from 'path'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. mixing es modules & commonjs here, is that recommended? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it not recommended? A common linting rule in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. webpack/webpack#4039 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This post is 2 years old and maybe babel / typescript already converts my import to es5 (can't check right now) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I came across this issue yesterday itself where I was mixing the two. This led to me getting some imports inside |
||
|
||
module.exports = async ({ config }) => { | ||
module.exports = async ({ config }: { config: any }) => { | ||
config.module.rules.push({ | ||
test: [/\.stories\.tsx?$/, /index\.ts$/], | ||
loaders: [ | ||
|
@@ -11,7 +11,7 @@ module.exports = async ({ config }) => { | |
}, | ||
}, | ||
], | ||
include: [path.resolve(__dirname, '../src')], | ||
include: [resolve(__dirname, '../src')], | ||
enforce: 'pre', | ||
}); | ||
return config; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,10 +12,12 @@ | |
"strict": true, | ||
"forceConsistentCasingInFileNames": true, | ||
"resolveJsonModule": true, | ||
"isolatedModules": true, | ||
"noEmit": true | ||
"isolatedModules": true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we also add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. isolatedModules: true
isolatedModules: false
Why should it be faster? What does this option even do? I tried to find more accurate documentation than "exports every file as separate module" Also keep in mind that this is an example app and depends on the decisions a project owner makes, not storybook. @ndelangen you added There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it could/should be faster because tsc doesn't need cross-file information to emit. Doesn't look like it makes any difference for us though. I would still recommend adding it in because it helps enforce rules around "Babel's single file emit architecture". See the caveats section at https://devblogs.microsoft.com/typescript/typescript-and-babel-7/ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just found out: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is why I suggest to add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm with @leoyli on this one, sounds like that's what we want? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm question:
Is there something I don't get or is this a misunderstanding? 🙂 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean, yes - it won't fail but at least your IDE will mark those imports red because they are missing - Isn't it confusing if you get red lines in your IDE? 🤔 or are internal packages not red because the IDE can handle this... |
||
}, | ||
"include": [ | ||
"src" | ||
], | ||
"exclude": [ | ||
"src/setupTests.js" | ||
] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like you might be able to remove some of this now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ndelangen you changed that, can you take a look at this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should remove if storyshots run successfully without it