Using Babel for TypeScript instead of tsc #5109

kroeder · 2018-12-27T18:31:17Z

What I did

Using babel as typescript compiler instead of tsc. This allows us to rely on only babel and also gives us the possibility to use a prop-type generator on compile time

storybook-safe-bot · 2018-12-27T18:34:59Z

	Fails
🚫	PR is marked with "do not merge" label.

Generated by 🚫 dangerJS against 2da70f6

codecov · 2018-12-28T00:39:21Z

Codecov Report

Merging #5109 into next will decrease coverage by 1.24%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##            next    #5109      +/-   ##
=========================================
- Coverage   38.3%   37.05%   -1.25%     
=========================================
  Files        649      649              
  Lines       9853    10181     +328     
  Branches     388      388              
=========================================
- Hits        3774     3773       -1     
- Misses      6019     6348     +329     
  Partials      60       60

Impacted Files	Coverage Δ
lib/core/src/server/common/babel.js	`0% <ø> (ø)`	⬆️
lib/ui/scripts/createDlls.js	`0% <ø> (ø)`	⬆️
app/angular/src/server/index.js	`0% <ø> (ø)`	⬆️
app/angular/src/server/build.js	`0% <ø> (ø)`	⬆️
lib/ui/paths.js	`0% <ø> (ø)`	⬆️
lib/theming/src/animation.ts	`100% <ø> (ø)`	⬆️
lib/ui/src/keyboard/platform.ts	`0% <0%> (ø)`	⬆️
lib/ui/src/keyboard/scanCode.ts	`0% <0%> (ø)`	⬆️
lib/ui/src/keyboard/keyCodes.ts	`0% <0%> (ø)`	⬆️
...s/storyshots-core/src/frameworks/angular/loader.js	`10% <0%> (-1.12%)`	⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e7cddc0...dc521ca. Read the comment docs.

.babelrc.js

Hypnosphi · 2019-01-02T13:34:30Z

Do we still need a separate compile-ts.js after this change?

…l-typescript TODO: need to migrate app/angular/server to TS in order to work with tsc

kroeder · 2019-01-04T13:17:41Z

@Hypnosphi I still need to do a couple of things like renaming files

After having some trobules with angular and decorators in general (so no blame on angular except the architectural choice of using decorators 😛 ) @ndelangen and I ended up using babel for js + ts except for angular that now uses tsc

I need to think about how to name those files

compile-js does babel
compile-ts does tsc stuff (not only for angular but also for generating declaration files d.ts)

Maybe we just rename them compile-babel and compile-tsc ?

ndelangen · 2019-01-04T15:28:58Z

Maybe we just rename them compile-babel and compile-tsc ?

Sounds good to me

kroeder · 2019-01-05T11:45:41Z

For the record: I had to migrate @storybook/angular in order to be able to compile it with tsc instead of babel because lots of stuff in this package is written in js

# Conflicts: # yarn.lock

# Conflicts: # .babelrc.js # app/angular/src/client/preview/render.js # package.json # yarn.lock

…orybooks/storybook into ts-migration/babel-typescript

kroeder · 2019-03-31T08:01:51Z

Can you please confirm my changes by using time yarn bootstrap --core without --reset? Installing node_modules is affected by network time and can vary on each run

A few tests based on my last commits

Tests are based on: time yarn bootstrap --core


Test-run with no changes
------------------------

real    2m17.811s
user    0m0.060s
sys     0m0.170s


Run only remove-dist + babel
----------------------------

real    0m53.101s
user    0m0.030s
sys     0m0.121s

After trying a lot I saw no potential in decreasing the babel-build time


Run only remove-dist + tscfy 
----------------------------

- No changes
real    1m5.509s
user    0m0.076s
sys     0m0.045s

- Adding "skipDefaultLibCheck": true
real    0m58.443s
user    0m0.075s
sys     0m0.107s

- adding "skipLibCheck": true besides "skipDefaultLibCheck": true
real    0m36.928s
user    0m0.075s
sys     0m0.106s


Final result after all optimizations
------------------------------------

real    1m30.265s
user    0m0.045s
sys     0m0.090s

kroeder · 2019-03-31T08:24:45Z

next is still faster

real    1m13.757s
user    0m0.030s
sys     0m0.230s

I can try a couple more things and check the differences between next and my branch but I start to question if this PR is a benefit for Storybook at all if we.

Cannot get rid of tsc because we want typings
Cannot get it faster due to some unknown reasons

Initially I wanted to move it to babel to have

Only one build system
Being able to generate react prop types by using typescript interfaces using https://github.com/milesj/babel-plugin-typescript-to-proptypes (I'm aware that this package has not much attention and I need to test if it works properly)

examples/angular-cli/package.json

benoitdion · 2019-03-31T13:43:42Z

addons/storyshots/storyshots-core/src/frameworks/angular/loader.js

  require.requireActual('core-js/modules/es6.promise');
+  // require.requireActual('core-js/es6/reflect');


looks like you might be able to remove some of this now

@ndelangen you changed that, can you take a look at this?

Should remove if storyshots run successfully without it

benoitdion · 2019-03-31T13:45:00Z

examples/cra-ts-kitchen-sink/tsconfig.json

@@ -12,10 +12,12 @@
    "strict": true,
    "forceConsistentCasingInFileNames": true,
    "resolveJsonModule": true,
-    "isolatedModules": true,
-    "noEmit": true
+    "isolatedModules": true


using isolatedModules in all our tsconfigs could improve tsc performance

should we also add declaration: false here?! I'm not sure what is the best way to make sure this one won't emit any typed files. If we want an emission, then this have to be set to false. See this issue on TS repo here.

isolatedModules: true

real 0m18.465s user 0m0.045s sys 0m0.045s

isolatedModules: false

real 0m18.345s user 0m0.030s sys 0m0.091s

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 isolatedModules: true - Were you just playing around with the options or was this on purpose?

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/

I just found out: TS5053: Option 'declaration' cannot be specified with option 'isolatedModules'. 🐙

That is why I suggest to add declaration: false together with isolatedModules, maybe with a comment? In my own project, I have tsc --isolatedModules --noEmit --watch for type checking, and tsc --declaration --removeComments --emitDeclarationOnly for type emission. The rest of them goes through babel.

I'm with @leoyli on this one, sounds like that's what we want?

Hm question:

You start yarn dev and yarn dev:tsc needs to create declaration files for other internal packages that rely on that package: How can I do declaration: false here

You start yarn bootstrap --core and internally packages again rely on the d.ts files of other packages: How can I do declaration: false

Is there something I don't get or is this a misunderstanding? 🙂

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...

lib/ui/src/keyboard/keyCodes.ts

scripts/compile-babel.js

benoitdion · 2019-03-31T13:49:35Z

scripts/compile-tsc.js

@@ -1,12 +1,20 @@
 /* eslint-disable no-console */
+// tslint:disable:no-console


still needed?

There are log outputs in the compile scripts that are marked red without

next PR: replace tslint with eslint everywhere, then it can be removed

scripts/compile-tsc.js

leoyli · 2019-03-31T16:15:10Z

Q: Why we want to inject react prop types? For people not using TS or for react-docgen?

shilman · 2019-04-01T01:59:03Z

@kroeder Seems like the performance is comparable to next for yarn bootstrap --core on my machine.

This PR:

real	1m52.390s
user	4m9.852s
sys	0m26.077s

next:

real	2m3.644s
user	3m45.516s
sys	0m56.512s

kroeder · 2019-04-01T17:38:33Z

Q: Why we want to inject react prop types? For people not using TS or for react-docgen?

@leoyli I'm not a react user but when I first started the whole typescript migration I had to write types twice:

Interfaces for TS users
PropTypes for non-TS users

These can get out of sync so my intention was to get rid of the manually written prop-types

leoyli · 2019-04-01T17:54:51Z

@kroeder, I can feel your pain... Thank your these great works and your explains.

I think it is really questionable on where we need to use prop-types. According to React official doc, it seems that the prop-types is really redundant if we decide to migrate the whole project into TS. But that is out of the scope here... 😓

ndelangen · 2019-04-01T18:22:36Z

examples/angular-cli/.storybook/webpack.config.ts

@@ -1,6 +1,6 @@
-const path = require('path');
+import { resolve } from 'path';


mixing es modules & commonjs here, is that recommended?

Is it not recommended? A common linting rule in ts is no-use-require or something like that. I can revert that if you want, it works though. I've never experienced any trouble using imports for path like that

webpack/webpack#4039 (comment)
what about this?

This post is 2 years old and maybe babel / typescript already converts my import to es5 (can't check right now)

I came across this issue yesterday itself where I was mixing the two. This led to me getting some imports inside default and some not. Not mixing them up helped me fix it. Although I am not quite sure about your usage.

ndelangen · 2019-04-01T18:32:20Z

if we decide to migrate the whole project into TS

That's not out of scope, it's our roadmap.

But never the less, users of storybook and addon-creators are not all migrating.
So by adding proptypes to our storybook components they'll be able to catch problems and help us detect issues at runtime too.

gaetanmaisse · 2019-04-01T19:30:40Z

tsconfig.json

-    "emitDecoratorMetadata": true,
-    "experimentalDecorators": true,    
-    "skipLibCheck": true,
+    "incremental": false,


Do you set incremental build to false because it is not working with babel for now?

I did that because it failed in a certain scenario.. I think calling yarn bootstrap --core twice
It should not use a cache here because it removes dist and compiles everything from scratch

I just forgot to add that option for yarn dev in watch mode
Maybe we can do a tsconfig.prod.json and a tsconfig.json to add transparency in the tsc build process

kroeder · 2019-04-01T20:54:46Z

If it is okay for everyone I'd like to do a feature-freeze for this PR and collect them in an issue for some sort of "Babel refactoring - Phase II" and only fix bugs or change requests that should be done before merging (if we decide to merge 🙂 ) - Is that okay?

Currently on the list:

Make yarn dev more efficient and less CPU consuming
Make tsc build transparent (by e.g. adding context based tsconfig.json, tsconfig.prod.json etc.)
Try adding isolatedModules: true when declarations can be false
Try using the prop type generator for babel https://www.npmjs.com/package/babel-plugin-typescript-to-proptypes
< Add your proposal here >

libetl · 2019-04-04T09:26:04Z

this one will have a conflict with my (very small) PR : https://github.com/storybooks/storybook/pull/6415/files

Do you think you can make the same change as in mine ? @kroeder
Thank Kai.

ndelangen · 2019-04-04T10:44:53Z

Let's merge @kroeder ?

vercel · 2019-04-04T11:44:29Z

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://monorepo-git-ts-migration-babel-typescript.storybook.now.sh

ndelangen · 2019-04-04T12:23:11Z

Congrats @kroeder

Epic work

Hypnosphi

Hi @kroeder! Do you remember why did you have to revert?
6e6df7c

kroeder · 2019-11-01T19:59:20Z

@Hypnosphi Mh no I'm not sure but can try to figure this out tomorrow. Does it cause any trouble?

Hypnosphi · 2019-11-01T20:28:14Z

Not really, just curious

yarn.lock

2fc1518

kroeder added ci: do not merge configuration babel / webpack maintenance User-facing maintenance tasks typescript labels Dec 27, 2018

kroeder self-assigned this Dec 27, 2018

kroeder mentioned this pull request Dec 27, 2018

TypeScript migration organization #5030

Closed

Using babel-typescript instead of tsc

1a5fa4b

kroeder requested review from Hypnosphi, igor-dv and ndelangen as code owners December 28, 2018 00:39

Added decorator support for babel

8543bf0

igor-dv reviewed Dec 29, 2018

View reviewed changes

.babelrc.js Outdated Show resolved Hide resolved

Angular now gets compiled by tsc, everything else is compiled by babe…

8c8775c

…l-typescript TODO: need to migrate app/angular/server to TS in order to work with tsc

kroeder requested a review from alterx as a code owner January 4, 2019 13:13

ndelangen added this to the next milestone Jan 4, 2019

kroeder added 2 commits January 5, 2019 11:26

Migrated app/angular/client to TS

a40589a

Migrated app/angular/server to TS; Still WIP

c5f4775

ndelangen self-assigned this Jan 5, 2019

ndelangen added 4 commits January 5, 2019 19:50

Merge branch 'next' into ts-migration/babel-typescript

468477d

# Conflicts: # yarn.lock

MAYBE fix build

2da70f6

Merge branch 'next' into ts-migration/babel-typescript

d351d87

# Conflicts: # .babelrc.js # app/angular/src/client/preview/render.js # package.json # yarn.lock

FIX circular dep

4be2657

kroeder requested a review from tmeasday as a code owner March 2, 2019 20:00

kroeder added 3 commits March 31, 2019 09:54

clean up babelrc

413438a

Add skipLibCheck to make things faster

ef57974

Merge branch 'ts-migration/babel-typescript' of https://github.com/st…

7237cd7

…orybooks/storybook into ts-migration/babel-typescript

benoitdion reviewed Mar 31, 2019

View reviewed changes

Resolved change requests

0b881e2

kroeder requested a review from CodeByAlex as a code owner April 1, 2019 17:17

kroeder requested a review from shilman April 1, 2019 17:21

ndelangen reviewed Apr 1, 2019

View reviewed changes

ndelangen approved these changes Apr 1, 2019

View reviewed changes

gaetanmaisse reviewed Apr 1, 2019

View reviewed changes

shilman approved these changes Apr 3, 2019

View reviewed changes

Add changes based on #6415

dc521ca

vercel bot deployed to staging April 4, 2019 11:31 View deployment

ndelangen merged commit 1fd38ca into next Apr 4, 2019

ndelangen deleted the ts-migration/babel-typescript branch April 4, 2019 12:22

Hypnosphi reviewed Nov 1, 2019

View reviewed changes

		require.requireActual('core-js/modules/es6.promise');
		// require.requireActual('core-js/es6/reflect');

		@@ -1,12 +1,20 @@
		/* eslint-disable no-console */
		// tslint:disable:no-console

		@@ -1,6 +1,6 @@
		const path = require('path');
		import { resolve } from 'path';

Using Babel for TypeScript instead of tsc #5109

Using Babel for TypeScript instead of tsc #5109

Conversation

kroeder commented Dec 27, 2018

What I did

storybook-safe-bot commented Dec 27, 2018 • edited Loading

codecov bot commented Dec 28, 2018 • edited Loading

Codecov Report

Hypnosphi commented Jan 2, 2019

kroeder commented Jan 4, 2019 • edited Loading

ndelangen commented Jan 4, 2019

kroeder commented Jan 5, 2019

kroeder commented Mar 31, 2019

kroeder commented Mar 31, 2019 • edited Loading

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

Choose a reason for hiding this comment

kroeder Apr 1, 2019 • edited Loading

Choose a reason for hiding this comment

benoitdion Apr 1, 2019 • edited Loading

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

Choose a reason for hiding this comment

kroeder Apr 2, 2019 • edited Loading

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

leoyli commented Mar 31, 2019

shilman commented Apr 1, 2019 • edited Loading

kroeder commented Apr 1, 2019

leoyli commented Apr 1, 2019

Choose a reason for hiding this comment

kroeder Apr 2, 2019 • edited Loading

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

ndelangen commented Apr 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kroeder commented Apr 1, 2019 • edited Loading

libetl commented Apr 4, 2019

ndelangen commented Apr 4, 2019

vercel bot commented Apr 4, 2019

ndelangen commented Apr 4, 2019

Hypnosphi left a comment • edited Loading

Choose a reason for hiding this comment

kroeder commented Nov 1, 2019

Hypnosphi commented Nov 1, 2019

storybook-safe-bot commented Dec 27, 2018 •

edited

Loading

codecov bot commented Dec 28, 2018 •

edited

Loading

kroeder commented Jan 4, 2019 •

edited

Loading

kroeder commented Mar 31, 2019 •

edited

Loading

kroeder Apr 1, 2019 •

edited

Loading

benoitdion Apr 1, 2019 •

edited

Loading

kroeder Apr 2, 2019 •

edited

Loading

shilman commented Apr 1, 2019 •

edited

Loading

kroeder Apr 2, 2019 •

edited

Loading

kroeder commented Apr 1, 2019 •

edited

Loading

Hypnosphi left a comment •

edited

Loading