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

Use Typescript instead of Javascript for React components #1001

Merged
merged 18 commits into from
Mar 27, 2017

Conversation

beagleknight
Copy link
Contributor

@beagleknight beagleknight commented Feb 17, 2017

🎩 What? Why?

In this PR I included two major improvements:

TLDR

Using Typescript instead of Javascript means having a more maintainable, scalable and robust code base. It is full compatible with React and Apollo.

Our test setup was a bit messy because we were using a lot of small libraries. I replaced everything with Jest, having a test suite 3x times faster.

From Javascript to Typescript

Before justify myself about the usage of Typescript I just want to clarify a few things. You can import any ES6 module written in Javascript into any Typescript module.

As Typescript is just a ES6 superset you can change the file extension from .js to .ts and start adding features.

Finally, let's see an example about migrating code from Javascript to Typescript:

Javascript

function add(a, b) {
  return a + b;
}

Typescript

function add(a: number, b: number): number {
  return a + b;
}

Why Typescript?

I have quite an experience with Javascript and I really love the language. When you start working with it in a serious project, far away of just using a bunch of jQuery plugins you realise it is a powerful language and the mere existence of Node.js proves it.

But it has a big dark side and a more darker history but we all already know that. Enters ES6 (with Babel) and everything starts looking shiny again. Nowadays we have a lot of powerful features but I am always gonna miss one: strict type checking for my code.

And then Typescript appeared (about 4 years ago) from the hand of Microsoft. I was used to overlook it, not paying too much attention because it wasn't widely use. Then I started working with React components and following good practises like propTypes. It felt like a hack and it wasn't a good solution in my opinion since the types were just verified at runtime. Finally I tried to write the same code in Typescript and felt so natural.

React is totally adapted to Typescript. Actually, there are a lot of libraries that are written in Javascript but you can find their types here: https://github.com/DefinitelyTyped/DefinitelyTyped
Since we are using Apollo and GraphQL we are already playing with a typed schema for our API. Luckily for us, Apollo developers thought about that and we can import our API schema into Typescript: https://github.com/AjuntamentdeBarcelona/decidim/pull/1001/files#diff-5eea45d1a1f49c33626671e137d492fe

In conclusion: using Typescript we ensure our code is more maintainable, scalable and robust.

Why Jest?

It wasn't my initial intention to include this in the same PR but I thought it was a good opportunity while migrating the code to Typescript.

We have a lots of tests for our components and they are just Unit tests. Since it it is just Javascript code and we have Node.js we don't need to run the code in a real browser. In order to run our tests in Node.js we have two main options: Jest or Ava.

I wanted to try Jest because it is from Facebook and our tests are mostly of our React components so it seems like a good combo. Not only is faster than Karma (we are talking about 3x faster), the setup is easier and it is compatible with Jasmine (the framework used to test).

Personally we can extract this part in another PR if we decide to not use Typescript for the moment.

📌 Related Issues

None

📋 Subtasks

  • Migrate components to typescript
  • Migrate tests to typescript
  • Add tslint
  • Add noImplicitAny and strictNullCheck compiler options

📷 Screenshots (optional)

None

👻 GIF

None

@andreslucena
Copy link
Member

Sorry but why this?
Where it's the issue and the discussion?
Now people need to learn React and also Typescript? What else it's needed to learn to be able to code this?

Personally, I think we're getting the developer entry barrier higher with this one.

@beagleknight beagleknight force-pushed the enhance/react/typescript branch from 510dfba to 99b5792 Compare February 20, 2017 08:07
@codecov-io
Copy link

codecov-io commented Feb 20, 2017

Codecov Report

Merging #1001 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1001      +/-   ##
==========================================
+ Coverage   97.14%   97.15%   +0.01%     
==========================================
  Files         421      421              
  Lines        7033     7033              
==========================================
+ Hits         6832     6833       +1     
+ Misses        201      200       -1
Impacted Files Coverage Δ
...trollers/decidim/proposals/proposals_controller.rb 95.91% <0%> (+2.04%) ⬆️

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 3e8ce7c...59560b7. Read the comment docs.

@josepjaume
Copy link
Contributor

@andreslucena sorry, we should have explained why this a bit more: We used last friday (as most fridays) to investigate, so @beagleknight felt like trying typescript and making a (personal) contribution. We didn't have a discussion, although @beagleknight has some strong points to using typescript (in which I agree, but we should obviously discuss this further).

Let's open an issue and talk about it!

@beagleknight
Copy link
Contributor Author

beagleknight commented Feb 20, 2017

Hi @andreslucena !

Sorry for the zero explanation. As @josepjaume commented before, I used the last friday to R&D a bit to migrate some code to typescript in order to verify if it plays nice with React. I created the PR before leaving the office and it was labeled as ready for an unknown reason 😄 . The PR is not ready at all and it needs a detailed explanation which I want to write very soon.

There are two strong points to use Typescript in favor of Javascript:

  • Type errors in compile time: React uses something called PropTypes for type validation in components. It's useful to verify if you are using the correct inputs in your components but just for run time so you can introduce an error very easily. I think they must deprecate the usage of PropTypes and people needs to start using Flow or Typescript for that. Javascript is not strongly typed and it will never be.

  • GraphQL and Schema type validation: We are using a graphql API for the comments backend and so far it is giving us a very good result. Since the schema is strongly typed, using Javascript means losing this and then we must rely in the PropTypes validation again. There is a tool created by the Apollo team which generates types from the graphql schema and our queries. Then we can use this types for type checking our components as well.

I need to elaborate both points a bit more and review the code in order to have a stronger opinion on this. This is not a priority and I will continue working on it in my free time. It looks very promising 😃 !

@andreslucena
Copy link
Member

(Just so you listen to the other bell)
Right now to modify decidim Javascript a developer needs to know:

  • jquery
  • React
  • Webpack
  • Yarn
    And now we want to add another language (non standard).

Decidim is getting too complex with all this javascript, for instance as a new developer to the framework I needed to search for a couple of minutes to get to this folder https://github.com/AjuntamentdeBarcelona/decidim/tree/master/decidim-comments/app/frontend/comments - this isn't the Rails Way and it isn't documented on no place.

Just for comparison, to do the same on Consul (https://github.com/consul/consul/blob/master/app/assets/javascripts/comments.js.coffee ), I need to know:

  • jquery
  • coffescript

@josepjaume
Copy link
Contributor

josepjaume commented Feb 20, 2017

@andreslucena I hear ya - but keep in mind we're only dealing with this on the comments module, for a good reason: Comments are a quite complex and justifies using powerful tools.

We have realtime comments out-of-the-box, nice polymorphic abstractions (whereas in consul the integration is ad-hoc in each controller), and a whole bunch of other niceties. Also, I believe having some space for experimentation is healthy for the project.

decidim is a complex project and it's natural that's difficult to grasp. That's why is highly modular - you should only know this if you're modifying/extending comments.

Also, to be fair, Yarn is just a drop-in replacement to npm and it's already became a standard, webpack is being integrated into rails. Also, the same learning curve argument could be used to avoid CoffeeScript but that didn't stop DHH xDD

@beagleknight
Copy link
Contributor Author

Hi @AjuntamentdeBarcelona/developers !

I finally finished this and I'm really happy about it 😄 ! It was a good session of R&D and very productive. I just want to know your opinion about this. Please, read carefully the PR description and understand my motivation in doing this.

@andreslucena
Copy link
Member

Personally I'm afraid about making steeper the learning curve for new developers.

@ahukkanen @Hilfe @zlworks @xredo as implementers, how do you see this?

@ahukkanen
Copy link
Contributor

To me TypeScript is a welcomed change and can help maintain larger apps (such as the comments module). I don't think TypeScript itself introduces that steep learning curve considering the developer already learned e.g. React.

@xredo
Copy link
Contributor

xredo commented Mar 20, 2017

The FrontEnd community seems to be embracing Typescript so I find appropriate having this discussion (we have also done it and we have adopted it).

As for the learning curve, I see Typescript as an easy to learn language if you previously have worked with JS. Syntax is derived from the Javascript syntax (not like what happened with Coffeescript for example) and all of their added features are optional (including the typings).

The only problems that we have found so far with Typescript are related to having a well configured build system behind, and integrating it with certain 3rd party libraries properly. However, this is something that shouldn't concern new developers as everything will be already set up and additional dependencies will be discussed with the core team.

My concern here is related to what features from Typescript will be enforced into the project and what will be optional. For example, will defining the type for every variable / method / interface be required before accepting a PR?

@mention-bot
Copy link

@beagleknight, thanks for your PR! By analyzing the history of the files in this pull request, we identified @josepjaume and @lastpotion to be potential reviewers.

@beagleknight beagleknight force-pushed the enhance/react/typescript branch from fc361c5 to 0d3419d Compare March 27, 2017 08:48
Copy link
Contributor

@josepjaume josepjaume left a comment

Choose a reason for hiding this comment

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

Could you update the README explaining how to re-generate types from the GraphQL schema and mentioning TypeScript?

@beagleknight
Copy link
Contributor Author

@josepjaume You are right! Updated!

#### Run tests

You can execute `yarn test` to run the javascript test suite or you can run `yarn test:watch` to listen for file changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove extra line

@@ -35,6 +35,38 @@ And then execute:
$ bundle
```

## How to contribute
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you also write a quick section explaining what's the use of this extension?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It already exists 😄

@beagleknight beagleknight force-pushed the enhance/react/typescript branch from db8c80f to 59560b7 Compare March 27, 2017 10:46
@beagleknight beagleknight merged commit 5557a21 into master Mar 27, 2017
@beagleknight beagleknight deleted the enhance/react/typescript branch March 27, 2017 12:12
@mrcasals mrcasals mentioned this pull request Mar 13, 2018
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.

9 participants