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

Using Babel for TypeScript instead of tsc #5109

Merged
merged 56 commits into from
Apr 4, 2019

Conversation

kroeder
Copy link
Member

@kroeder kroeder commented Dec 27, 2018

Issue: #5030

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
Copy link
Contributor

storybook-safe-bot commented Dec 27, 2018

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

Generated by 🚫 dangerJS against 2da70f6

@codecov
Copy link

codecov bot commented Dec 28, 2018

Codecov Report

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

Impacted file tree graph

@@            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 Outdated Show resolved Hide resolved
@Hypnosphi
Copy link
Member

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 kroeder requested a review from alterx as a code owner January 4, 2019 13:13
@kroeder
Copy link
Member Author

kroeder commented Jan 4, 2019

@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
Copy link
Member

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

Sounds good to me

@ndelangen ndelangen added this to the next milestone Jan 4, 2019
@kroeder
Copy link
Member Author

kroeder commented Jan 5, 2019

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

@ndelangen ndelangen self-assigned this Jan 5, 2019
# Conflicts:
#	.babelrc.js
#	app/angular/src/client/preview/render.js
#	package.json
#	yarn.lock
@kroeder kroeder requested a review from tmeasday as a code owner March 2, 2019 20:00
@kroeder
Copy link
Member Author

kroeder commented Mar 31, 2019

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
Copy link
Member Author

kroeder commented Mar 31, 2019

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

examples/angular-cli/package.json Outdated Show resolved Hide resolved
require.requireActual('core-js/modules/es6.promise');
// require.requireActual('core-js/es6/reflect');
Copy link
Member

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

Copy link
Member Author

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?

Copy link
Member

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

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

Choose a reason for hiding this comment

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

using isolatedModules in all our tsconfigs could improve tsc performance

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@kroeder kroeder Apr 1, 2019

Choose a reason for hiding this comment

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

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?

Copy link
Member

@benoitdion benoitdion Apr 1, 2019

Choose a reason for hiding this comment

The 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/

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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? 🙂

Copy link
Member Author

@kroeder kroeder Apr 2, 2019

Choose a reason for hiding this comment

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

lib/ui/src/keyboard/keyCodes.ts Show resolved Hide resolved
scripts/compile-babel.js Outdated Show resolved Hide resolved
scripts/compile-babel.js Show resolved Hide resolved
@@ -1,12 +1,20 @@
/* eslint-disable no-console */
// tslint:disable:no-console
Copy link
Member

Choose a reason for hiding this comment

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

still needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

scripts/compile-tsc.js Show resolved Hide resolved
@leoyli
Copy link
Contributor

leoyli commented Mar 31, 2019

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

@shilman
Copy link
Member

shilman commented Apr 1, 2019

@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 kroeder requested a review from CodeByAlex as a code owner April 1, 2019 17:17
@kroeder kroeder requested a review from shilman April 1, 2019 17:21
@kroeder
Copy link
Member Author

kroeder commented Apr 1, 2019

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
Copy link
Contributor

leoyli commented Apr 1, 2019

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

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

Choose a reason for hiding this comment

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

mixing es modules & commonjs here, is that recommended?

Copy link
Member Author

@kroeder kroeder Apr 2, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

The 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)

Copy link
Contributor

Choose a reason for hiding this comment

The 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 default and some not. Not mixing them up helped me fix it. Although I am not quite sure about your usage.

@ndelangen
Copy link
Member

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.

"emitDecoratorMetadata": true,
"experimentalDecorators": true,
"skipLibCheck": true,
"incremental": false,
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Member Author

kroeder commented Apr 1, 2019

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
Copy link
Member

libetl commented Apr 4, 2019

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
Copy link
Member

Let's merge @kroeder ?

@vercel
Copy link

vercel bot commented Apr 4, 2019

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 ndelangen merged commit 1fd38ca into next Apr 4, 2019
@ndelangen ndelangen deleted the ts-migration/babel-typescript branch April 4, 2019 12:22
@ndelangen
Copy link
Member

Congrats @kroeder

Epic work

Copy link
Member

@Hypnosphi Hypnosphi left a comment

Choose a reason for hiding this comment

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

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

@kroeder
Copy link
Member Author

kroeder commented Nov 1, 2019

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

@Hypnosphi
Copy link
Member

Not really, just curious

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.