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

[docs] AppBar and Textfield demos in TypeScript #13229

Merged
merged 46 commits into from
Jan 31, 2019

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Oct 13, 2018

It's very unpolished at the time but I wanted to get some feedback before deep diving.
Feature is only available in the app bar demos and text field demos

Motivation

Implementation

This is an early draft. There are some issues that are not adressed yet

Why babel?

The only runtime difference right now is the usage of createStyles which is just the identity function but it's noise for js users. It would be nice to strip this from production code. Typescript tansformer API is either not documented well or the ts team has no goal in unifying this API (see microsoft/TypeScript#14419) hence we use a babel plugin.

I didn't look into that but it might be a good idea anyway to have a webpack/babel plugin that removes this function. I don't know if babel can strip identity functions or if something like prepack is required for that.

babel-plugin vs. ts-transformer

  • babel-plugin allows usage in configs, ts-transformers are only available for programmatic compilation
  • babel is pretty good with whitespace preservation, ts is not (Emit does not preserve empty lines microsoft/TypeScript#843)
  • depending on project setup only one is applicable (users with ts only might want a transformer which integrates well with at-loader and ts-loader, babel plugin integrates well if users compile with babel-preset-ts)

Issues

  • code is unformatted. we need to pipe the output to prettier resolved thanks to babel-plugin-generator-prettier Using retainLines in generatorOpts in babel with some manual regex fixing helps fix most of the problems.
  • [ ] propTypes need a any cast edit: will leave it at that; they are way to strictly typed atm
  • [ ] demos need to be written in js for dev hot reloading yarn docs:typescript --watch in parallel to yarn docs:dev
  • do we check in CI if the files are synced (not done for propTypes and ts declarations) edit: add outdatedTS to mark some demos as not-synced. Can later decide if this means that deploy is blocked, hide the switch or just display the warning (current impl) We have docs/ignore-ts-demo.json to suppress CI failures.
  • [ ] write demos only in ts and transpile it when building the docs bundle? (might loose contributors) Maintenance overhead is fine for hooks which requires actual implementation port. Porting from js to ts is for most PRs just c&p.

/cc @mui-org/core-contributors and especially @pelotom as the go2 ts member

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 13, 2018

@eps1lon It's an interesting initiative!
I believe you have already thought of the main cons. How can we afford the operational overhead of maintaining two versions of each demo? Maybe we could convert few, add one GA tracking event and see if they are used?

@mbrookes
Copy link
Member

frequent support requests how to use demos in typescript

"citation needed" (sorry, couldn't resit 😉), but really?

typescript documentation is just one guide that can be missed easily

Thank goodness for search! 😄

And +183 lines for what is, in essence, a 3 line diff?

Honestly, if a user can't type the demos after reading the docs, they're never going to be able to type their own code. (I say this as someone whose ts is even worse than their js, so not a criticism, just a blunt fact).

@eps1lon
Copy link
Member Author

eps1lon commented Oct 13, 2018

frequent support requests how to use demos in typescript

"citation needed" (sorry, couldn't resit ), but really?

Always valid. I wanted to push this quick and was a bit lazy. Those are examples that would be addressed by this: #12486, #12946, #12709, #13004, #13218, #13372, #13396 and some gitter conversations.
These are of course not literal but they all address usage questions.

And +183 lines for what is, in essence, a 3 line diff?

Are you referring to the changed demo? I thought I addressed those concerns with the Proof of Concept tag and repeating that this is unpolished at the moment. The end goal is to have no diff at all.

Honestly, if a user can't type the demos after reading the docs, they're never going to be able to type their own code. (I say this as someone whose ts is even worse than their js, so not a criticism, just a blunt fact).

Let me rephrase my motivation (also in response to @oliviertassinari ). I think our typescript declarations are not tested well. I wanted to put some work into writing new tests but like with every test I feared that this might result in suites that don't really stress test or don't reflect real world use cases.

I believe that the demos are the strongest part of this documentation and they are already used for regression tests so why not also use them to test our declarations. But why just hide those in test files when we can also put them into the docs and showcase how good or bad this library works with typescript. In essence our tests are literally part of the documentation.

@oliviertassinari
One thing I like about this implementation is that we can gradually port the demos to typescript. I would say maybe our 3 most popular demos and then see how often people look at the typescript version. Maybe also research how often mui is used with typescript. If people are not interested I can still move them into the typescript test folders and use them as tests without enhancing our docs.

Edit:

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 14, 2018

@oliviertassinari
One thing I like about this implementation is that we can gradually port the demos to typescript. I would say maybe our 3 most popular demos and then see how often people look at the typescript version. Maybe also research how often mui is used with typescript. If people are not interested I can still move them into the typescript test folders and use them as tests without enhancing our docs.

I'm all in for the scientific approach 🔬. It starts with an intuition, it leads to a quick experimentation and finishes with an analysis of the results. repeat ♻️ .

@pelotom
Copy link
Member

pelotom commented Oct 15, 2018

Directionally I think this is a great idea, as would be porting the unit tests over to TypeScript (@rosskevin took a stab at this a year or so ago but we never got it over the finish line). The more validation code we can write in TypeScript, the more we can simultaneously validate the implementation as well as the fact that the types correspond to that implementation.

@eps1lon
Copy link
Member Author

eps1lon commented Oct 16, 2018

The current implementation generates for the very simple (concerning types) demo BottomAppBar the same JS from TS. I updated the notes about the babel plugin.

https://deploy-preview-13229--material-ui.netlify.com/demos/app-bar/#bottom-app-bar is a preview of the JS/TS switch. Currently there is a global one in the docs app bar.

The global switch looks out of place and in the current form might not be useful since we won't have much examples in TS to begin with. It's just a first draft and I'm pretty certain it's not gonna make it.

The switch per demo looks good though in my opinion and just need some UX improvements since tooltips won't work with disabled elements. State of the logo vs. code might be confusing. I want to signal that clicking it will toggle between JS and TS but then the JS logo is display when TS code is shown. Probably some toggle button group with colorized logo if activated is better.

@eps1lon eps1lon force-pushed the docs/examples-in-typescript branch from 775fa48 to f6b0001 Compare October 17, 2018 14:17
@oliviertassinari oliviertassinari added the docs Improvements or additions to the documentation label Oct 17, 2018
@eps1lon eps1lon force-pushed the docs/examples-in-typescript branch 2 times, most recently from 60b2061 to 0b8e392 Compare October 23, 2018 05:19
@eps1lon
Copy link
Member Author

eps1lon commented Oct 23, 2018

This is now implementation wise complete. I updated the initial post.

It's only enabled for the app bar right now. I would like to port another component demo. @oliviertassinari Could you tell me the most or 2nd most popular demo?

@mbrookes
Copy link
Member

TextField 🥇, Button 🥈, AppBar 🥉.

@eps1lon eps1lon force-pushed the docs/examples-in-typescript branch 3 times, most recently from 5386445 to 6d9e13d Compare October 26, 2018 18:49
@oliviertassinari
Copy link
Member

@eps1lon Should I review this pull request ?

@eps1lon eps1lon force-pushed the docs/examples-in-typescript branch from 6d9e13d to d86a071 Compare October 30, 2018 09:55
@eps1lon
Copy link
Member Author

eps1lon commented Oct 30, 2018

@oliviertassinari That would be nice.

There are still some issues with codesandbox but I think switching to create-react-app@2.1 instead of react-scripts-ts might resolve those.

eps1lon added a commit to eps1lon/material-ui that referenced this pull request Oct 30, 2018
Followup on mui#13428 with courtesy of mui#13229
@eps1lon eps1lon force-pushed the docs/examples-in-typescript branch 3 times, most recently from ec63de4 to 4135e72 Compare November 5, 2018 09:37
@eps1lon eps1lon changed the title POC: demos in typescript [docs] Demos in TypeScript Nov 5, 2018
@eps1lon eps1lon force-pushed the docs/examples-in-typescript branch from 4135e72 to c915752 Compare November 16, 2018 11:52
commit 2035e6daa1ceba1c78d3e8740af8d5f3066bc898
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Mon Nov 5 10:16:31 2018 +0100

    [docs] Fix undefined raw js

commit 9ab3761e49d8682530543f1e6808d46cd955f32a
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Mon Nov 5 10:07:38 2018 +0100

    [docs] Enable textfield ts demos

commit 8271f5cff4b3f6fe31b564a3f98ed69d8c09f3d6
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Tue Oct 30 11:58:29 2018 +0100

    [docs] Port mui#13428 to typescript

commit 8b808886235a3690b01335a5d3a6132209b87483
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Fri Oct 26 17:55:02 2018 +0200

    [docs] Add typescript demos for TextField

    Takeaway:
    - inputProps, inputComponent is badly typed (needs generics though)
    - computed property keys support is bad in typescript

commit 5ab9c988d259cd039e5b2735a47c1c89c9761eec
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Tue Oct 23 13:36:56 2018 +0200

    [docs] Use esModuleInterop import conistently

commit c9bca0824c0f81fe114f8976bc10464de94306b2
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Tue Oct 23 13:35:32 2018 +0200

    [docs] Fix IE 11 compat issues

commit b4dd772ee990dd0f9a042a8721aa7a665ded7fce
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Tue Oct 23 13:33:44 2018 +0200

    [docs] Remove dead code

commit f0a23cffb7a8e2c4faf5c2b4182919efb74d4bdd
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Tue Oct 23 08:21:07 2018 +0200

    [ci] Fix job order

    Running git diff after yarn build will exit with 1 because the size
    snapshot was updated.

commit 114022bd6d1ac98af330f6e7b57ba03062d120ff
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Mon Oct 22 21:32:54 2018 +0200

    [docs] Disabled stackblitz for TS demos

commit 7995d67cd09f08541f388124d535452054518d75
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Mon Oct 22 17:47:14 2018 +0200

    [docs] Add guide about ts demos

commit 0c65724f14c4de21ced8ce4e8ff91b2edce3b6d5
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Mon Oct 22 17:09:39 2018 +0200

    [docs] Sync demo changes from 06967ec

commit 7a0861d855d15bf281fde271e0028d0b1ba9dbb1
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Mon Oct 22 15:35:03 2018 +0200

    [docs] Allow require default interop for ts demos

    Seems like codesandbox still has issues with this. Gonna investigate if
    we can fix this upstream

commit c8a143e20735be14f692477fab40cd3d4a3040df
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Mon Oct 22 15:34:10 2018 +0200

    [docs] Support types for scoped packages

commit 48a425d19a53e77fa97692980e364f503491a91b
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Sat Oct 20 18:02:05 2018 +0200

    [babel-plugin-unwrap-createStyles] Fix fixtures being transpiled with workspace config

commit 03c1ac9d7575c9dc79e8253578dda60f39e33375
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Fri Oct 19 12:21:59 2018 +0200

    [docs] Create ts specific codesandbox

    - needs babel plugin for synthetic default imports

commit cdc393185a823bb684ff34be23f20d4897db8962
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Thu Oct 18 18:39:40 2018 +0200

    [docs] Fire analytics event when clicking codeLanguage

commit 6865f6488c27c1353cae108410d94a165343414b
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Thu Oct 18 18:15:05 2018 +0200

    [docs] Add ts version for all app bar demos

    - Fixes menu not closing in PrimarySearchAppBar
    - removes some dead code from undefined classnames

commit a189a173a371e785916b8797283798e45d4743d8
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Thu Oct 18 17:12:44 2018 +0200

    [ci] Check compiled ts demos are equal to js demos

commit 95706fdaaa1b6a56da239250b515a36459c87b8b
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Wed Oct 17 20:51:02 2018 +0200

    [docs] Remove debug config

commit a90c76a322104787837c1646fc400e84cdc77e6c
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Wed Oct 17 20:48:56 2018 +0200

    [docs] github link and copy code depending on selected code language

commit fafa284f397e66b0075a29823debc1c8dbd6b1fb
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Wed Oct 17 18:14:49 2018 +0200

    [docs] Add feature flag for language switch

    Enabled on a per-component basis.

commit 9ef68446d7e42347d30fc796d34f23f15eaf4405
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Wed Oct 17 18:10:20 2018 +0200

    [docs] Add the ability to mark ts demos as outdated

    This should be used in-sync with ignored demos in transpilation.

commit a7033f203278d6e462a016190790a3609aabded5
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Wed Oct 17 17:18:10 2018 +0200

    [docs] Add the ability to ignore ts demos from transpilation

    This can be useful to defer syncing them upwards from js.

commit e7c19cb1f264bf9ec9b6884650b4e3ad8e6942f8
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Wed Oct 17 16:22:36 2018 +0200

    Fix formatting errors

commit ea9a6de935394d0c61eff974a8575165ae0ed1aa
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Wed Oct 17 10:33:23 2018 +0200

    [docs] Disable ts switch if not ts version is available

commit 5a58595c26108e8ccd18eacf3b16ae2ed40de0a5
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Wed Oct 17 09:19:53 2018 +0200

    [docs] Remove global code language switch

    Revisit this if we reach critical mass on ts docs

commit aecb12fdf96aa761e543ba284d55ffd11a7970f1
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Wed Oct 17 09:15:52 2018 +0200

    [docs] Improve code language switch

commit 0a587ec8b0f97dfc05352856f7dec1c081e5275e
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Tue Oct 16 23:37:40 2018 +0200

    Fix CI errors

commit 82921d138f21272cef11c040ea7a578584563120
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Tue Oct 16 23:20:36 2018 +0200

    [docs] Add local language switch and add language logos

    - color needs rework: it looks a little bit out of place, bad contrast

commit 49861e54f5b5d2e54cac672c4f5a3377731e6255
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Tue Oct 16 20:45:25 2018 +0200

    [docs] Language switch via redux

    It's pretty slow at the app level. While it is nice to have ts
    permanently enabled while browsing docs a switch at demo
    level might be better suited.

commit 6a06a060ad17f44dab0d1cc49996455b37f8a2fc
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Tue Oct 16 19:14:55 2018 +0200

    [docs] Another whitespace fix attempt

commit fffed851504e587f2abadd6353318eaab9791fdb
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Tue Oct 16 18:46:46 2018 +0200

    [docs] revert to function declaration

    propTypes in typescript are to strict

commit ccb62823cbbacb5da109eb5275277a4ebf797533
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Tue Oct 16 18:32:38 2018 +0200

    [core] Bump @types/react

commit d3ac480d1966485b1c3d22672797110c26a82ef5
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Tue Oct 16 18:25:06 2018 +0200

    [babel-plugin-unwrap-createStyles] Fix lint errors

commit 1998640e939bfb57e27c265c2e338ef6707bae5a
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Tue Oct 16 18:21:45 2018 +0200

    [docs] Remove diff in trailing whitespace

commit 912c2a8d060ff3de9a70eaefa62c011f2bdbc90d
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Tue Oct 16 18:20:13 2018 +0200

    [docs] Write formatted js from ts demos

commit 4dcd51f50252823ee5f40dd9e53bfd6821786d5f
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Tue Oct 16 17:28:08 2018 +0200

    [core] Sync tslint with eslint concerning trailing-whitespace

commit 05f3f3d78e333ed740f447a2e20724cc1624c363
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Tue Oct 16 13:59:47 2018 +0200

    [core] docuent babel-plugin vs. ts-transformer

commit 691a036456486b4cea411d435392746ad714817d
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Tue Oct 16 11:03:08 2018 +0200

    [docs] Add babel plugin to unwrap createStyles calls

    This essentially strips them from the bundle

commit a02ffd64a1defb4298030fd2d162c4a71c65b09a
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Sat Oct 13 16:02:11 2018 +0200

    Move ts files next to js files

commit 32a301a8eb2d0ed5e3b902313d5895dabcfbca32
Author: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date:   Sat Oct 13 14:35:14 2018 +0200

    Working example without type checking and bad folder structure
@eps1lon eps1lon force-pushed the docs/examples-in-typescript branch from c915752 to 33dfce0 Compare November 21, 2018 11:18
@mbrookes
Copy link
Member

@mbrookes What's the state of the toggle button integration?

Ah, sorry about that, didn't mean to derail this PR being merged.

The toggle button integration is done, but I ran into a roadblock on the MDv2 re-style. Let me know when you're done iterating, and I'll push it in its current form.

@eps1lon
Copy link
Member Author

eps1lon commented Jan 29, 2019

@mbrookes You can go ahead and push your changes.

@eps1lon eps1lon changed the title [docs] Demos in TypeScript [docs] AppBar and Textfield demos in TypeScript Jan 31, 2019
@eps1lon
Copy link
Member Author

eps1lon commented Jan 31, 2019

@mbrookes Thanks for helping with the language switch UI.

PR looks fine to me now. Maybe we can improve the whitespace issue later on but I'd rather not discuss formatting anymore in the day and age of prettier.

}

return (
<React.Fragment>
Copy link
Member

Choose a reason for hiding this comment

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

Do we need a fragment?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's just an artifact from the previous iteration that returned multiple buttons.

@@ -20,6 +20,8 @@ const styles = theme => ({
});

class ComposedTextField extends React.Component {
labelRef = null;
Copy link
Member

Choose a reason for hiding this comment

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

The bad flow times are back, adding code that does nothing just to make the type checker happy.

Copy link
Member Author

Choose a reason for hiding this comment

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

Knowing something does nothing, got it.

const reqSource = require.context(
'!raw-loader!../../docs/src/pages/demos/app-bar',
false,
/\.(js|tsx)$/,
Copy link
Member

Choose a reason for hiding this comment

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

Will we be able to run the TypeScript version at some point? Looking at the imports, it seems we are only using the .tsx file for displaying the source of the demo.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe. Not sure if we can just use the typescript preset for js files.

@oliviertassinari oliviertassinari merged commit 4422ce8 into mui:master Jan 31, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 31, 2019

@eps1lon Boom! 🦄🔥 with #14356 we gonna know exactly the market share of these TypeScript demos :).

@eps1lon eps1lon deleted the docs/examples-in-typescript branch January 31, 2019 15:32
@mbrookes
Copy link
Member

mbrookes commented Feb 1, 2019

Maybe we can improve the whitespace issue later on but I'd rather not discuss formatting anymore in the day and age of prettier.

I was talking about whitespace in the design sense. e.g. marginBottom added the v2 spec toggle buttons to separate them from the code container. The drop-shadow sort of does that for the v1 spec toggle buttons.

@eps1lon
Copy link
Member Author

eps1lon commented Feb 1, 2019

Maybe we can improve the whitespace issue later on but I'd rather not discuss formatting anymore in the day and age of prettier.

I was talking about whitespace in the design sense. e.g. marginBottom added the v2 spec toggle buttons to separate them from the code container. The drop-shadow sort of does that for the v1 spec toggle buttons.

This was targeted at https://github.com/mui-org/material-ui/pull/13229/files#r248695540

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants