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

react native Typescript transform #3209

Merged
merged 11 commits into from
Apr 4, 2018

Conversation

cliedeman
Copy link
Contributor

Issue:

What I did

Added a flag to the react-native storybook cli to allow passing a custom config to the metro bundler

How to test

Build the project and copied the compiled storybook-start.js, also created a sample project.

Is this testable with jest or storyshots?
No

Does this need a new example in the kitchen sink apps?
Yes, added

Does this need an update to the documentation?
Docs added

If your answer is yes to any of these, please make sure to include it in your PR.

@codecov
Copy link

codecov bot commented Mar 23, 2018

Codecov Report

Merging #3209 into master will decrease coverage by 1.22%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3209      +/-   ##
==========================================
- Coverage   36.84%   35.62%   -1.23%     
==========================================
  Files         458      472      +14     
  Lines       10028    10126      +98     
  Branches      904      955      +51     
==========================================
- Hits         3695     3607      -88     
- Misses       5763     5891     +128     
- Partials      570      628      +58
Impacted Files Coverage Δ
app/react-native/src/bin/storybook-start.js 0% <0%> (ø) ⬆️
lib/core/server.js 0% <0%> (-100%) ⬇️
addons/knobs/react.js 0% <0%> (-100%) ⬇️
lib/core/src/server/config/env.js 0% <0%> (-80%) ⬇️
.../core/src/server/config/defaults/webpack.config.js 0% <0%> (-69.24%) ⬇️
lib/core/src/server/utils.js 0% <0%> (-40.48%) ⬇️
lib/core/src/server/build-static.js 0% <0%> (-23.08%) ⬇️
lib/core/src/server/build-dev.js 0% <0%> (-20%) ⬇️
app/react/src/demo/Welcome.js 63.15% <0%> (-9.26%) ⬇️
.../ui/src/modules/ui/components/layout/dimensions.js 15.62% <0%> (-6.96%) ⬇️
... and 132 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 8bc8f9a...098c664. Read the comment docs.

@@ -84,6 +85,11 @@ if (!program.skipPackager) {
}

let cliCommand = 'react-native start';

if (program.metroConfig) {
cliCommand += ` --config ${program.metroConfig}`;
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to make it work with haul? This variable is reassigned when using it (see below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For haul according to the docs you just modify the generated webpack.haul.js file for typescript

@@ -0,0 +1,54 @@
[ignore]
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to configure Flow in a Typescript project?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

part of the react-native boilerplate. will delete

@Hypnosphi
Copy link
Member

Please add an option for bootstrap script: https://github.com/storybooks/storybook/blob/master/scripts/bootstrap.js

@cliedeman
Copy link
Contributor Author

Bootstrap script option added

Copy link
Member

@igor-dv igor-dv left a comment

Choose a reason for hiding this comment

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

TBH, I don't see why we need to have a full react-native-typescript example as part of the monorepo.

This is just an example of the compatibility between react-native & storybook & typescript, in this manner we may have all the variations of examples with angular/vue/polymer/typescript/flow/(jquery 😓) but it will be very hard to maintain.

So it can be

  1. A nice blog post
  2. A separate repo in the organization
  3. A seprate repo in your github account (referenced from the nice blog post).

@cliedeman
Copy link
Contributor Author

Hello, happy to go either route.

"Does this need a new example in the kitchen sink apps?" is a bit open-ended so I just created one anyway.

If/when this gets merged and released I am happy to do a blog post with a sample repo. It's a lot of fanfare for what is actually just adding a metro bundler flag.

Ciaran

@danielduan
Copy link
Member

@igor-dv I think this provides us with a test of TS compatibility with RN along with a much more thorough check of the vanilla RN as well. Our current vanilla example doesn't do much at all.

@danielduan danielduan changed the title Typescript transform react native Typescript transform Apr 2, 2018
Copy link
Member

@danielduan danielduan left a comment

Choose a reason for hiding this comment

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

examples/react-native-typesript/.gitignore
the typescript in the folder path is misspelled.

we should also set it up to run in CI so that we can make sure it's working and up to date.

@cliedeman
Copy link
Contributor Author

@danielduan Thanks, renamed

@igor-dv I have added storyshots too now so it should be more useful.

@danielduan Would running the storyshots (npm test) be good enough for CI? Not sure what else we can test via CI

Ciaran

Copy link
Member

@igor-dv igor-dv left a comment

Choose a reason for hiding this comment

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

I am still convinced that it's an overkill for this repo.

I would rather create a separate Monorepo "storybook/use-cases" and will add there all the relevant examples. For example storybook-with-css-modules, storybook-with-rn-ts and so on.

IMO if something is not vital for the project/brand, it should be in a separate place with its own CI and tests. Think of webpack-contrib, jest-contrib and others.

@@ -0,0 +1,58 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

please add here

"lerna": {
    "disabled": true
  },

Otherwise, this project will participate in the ts distribution

@ndelangen
Copy link
Member

I tend to agree with @igor-dv on the need for a storybooks/storybook-examples repo...

I'd like to discuss this during a roadmap meeting. We can extract it out later, so I do not find this blocking for this PR.

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.

5 participants