adonisjs / discussion

Discussing about new features and sharing random thoughts: โš ๏ธ Not every request will be accepted
51 stars 5 forks source link

Version 5 Static Typing Discussion #65

Closed jakesylvestre closed 4 years ago

jakesylvestre commented 6 years ago

I was procrastinating and ended up spending way to much time writing this ๐Ÿ˜†. Anyway because of length I've broken it up a bit. I've also gone ahead and moved this to a seperate issue as per @tlgreg here and moved this into a separate issue.

As per the discussion on adonisjs/adonis-framework#871 and here I think there's a few questions here that need to be looked at within @thetutlage's constraints.

  1. Should the framework be rewritten in typescript? to make use of use() while maintaining a public js api (ember style)
  2. If not, how can the framework best support typescript use cases
  3. Should the framework support flow and how should that be acheived?

I found some pretty nice advantages to typing in the app itself that I think are worth highlighting while setting up a adonis flow project

Adonis Flow Example

tl;dr, I put together jakesyl/adonis-flow-example (what I'm using now to use flow in adonis.)

I think flow would be really awesome to have. As per adonisjs/adonis-framework#907 I actually got a hacky version setup here: jakesyl/adonis-flow-example. The projects pretty immature (I just took an old commit from before I started building anything custom so there's not too much there) but it should give you a good idea of my current setup.

Essentially, I have a babel transpiler running to make the flow syntax valid and babel-plugin-transform-flow-strip-types to make it runnable by adonis serve --dev. So I transpile my source code (in a directory called appFlow to app and it gets run by adonis.

It also might be possible to run flow natively on adonis using type annotations in comments (although I haven't used this yet & don't find it to be clean). The best way to approach this (albeit without runtime type checking) might be by adding flow to the jsdoc. This would negate the need for babel.

Advantages I've found from running Adonis with Flow

tl;dr: easy to implement runtime type checking, typed properties on models

One of the interesting add-ons here that I think is worth mentioning if @adonisjs is considering a flow/typescriptimplementation is tcomb which allows for runtime type checking and for the runnable code not to have Flow included, but run static type checking on the code in appFlow.

The biggest advantage I've found so far is the ability to specify the properties of a Model, especially since adonis is DDL/not supporting mongo/graphql for the foreseeable future.

Wether or not typing makes sense

Lessons from Laravel

tl;dr: The Laravel community has been discussing

As far as rewriting the core with type-hinting, I actually think we can get some good guidance from the @laravel community here. It is worth noting that there are some pretty significant differences here: type hinting (in the case of php 7 and hack vs strong typing (in the case of typescript, not flow which alone does not do runtime checking).

They didn't end up implementing it for a few reasons. The first is that for the "most part if someone passes the invalid type, the call will fail somewhere during the stack". This doesn't make a lot of sense to me, I think failing earlier/not failing at all because of static analysis is going to be better.

Other criticisms include the fact that doc blocks already serve as typing without creating as much visual debt that an ide will do static analysis on them. Both laravel and adonis are thoroughly documented via docblocks. The issue is this assumes the developer has an ide that can catch wrong docblocks on a users code and that dependencies (including adonisjs) are correct/up to date which just isn't realistic with the variance in ides/big codebases where it's easy to overlook when merging a PR on github.

There are also just some methods that are hard to type for a very low improvement in debuggability. @JeffreyWay (@laracasts founder) has argued (citing a co-architect of PHP) that one of the core advantages of php (and, for our purposes other weakly typed languages) is simplicity. That simplicity comes in the form of being able to use typing where its needed and the majority of the time- it's simply not needed. @taylorotwell (@laravel author) has even said that they don't really reduce the amount of tests you need using a sizeable http library test suite as an example.

Many in the @laravel community have disagreed with @taylorotwell this. The best counterargument I've found so far was in response to a video by @JeffreyWay showing why he agreed with @taylorotwell's assesment. The video, as one community member who disagreed with him put it, showed him repeteadly removing type hints, making sure the code still ran and deeming it was less to look at and therefore would decrease visual debt without increasing tech debt. The author counterargues by pointing out how the static analysis increases test robustness by ensuring type coercion doesn't make tests show false positives. The (php) example the author gives is making sure null are not cast to true/false values causing tests to silently fail. Plus many of the lower ROI methods anti-static typing users are reffering to require little effort to statically type.

Another compelling argument against the assertion that static typing creates visual debt is pretty simple and that It's quite easy to hide/truncate types. Others just don't buy that visual debt is that big of a deal.

Other factors

I also think (as I mentioned above) that having the ability to have explicit typed properties on lucid models will dramatically increase the speed of development (even if this is all that ends up getting supported through some kind of flow implementation) and make it easier to code by autosuggesting fields/having fields available visually in the model. getters might be a good substitute here for autocomplete, but I think the typed fields are pretty powerful. I'm not sure if there's a way to duplicate that functionality in typescript/vanilla js but that might be something interesting to look into for 5.0. On a side note, Laravel does not do this but there's a very popular tool called laravel-ide-helper that generate docblocks have pretty heavily supplemented that.

There are a bunch of other benefits that weren't outlined by laravel since they weren't framework specific. While there is visual debt, I'd argue in a lot of places seeing the types explicit is very helpful. It's a lot easier than manually valdating arguments passed to functions .which cleans up a lot of boilerplate error code. For starters it could probably help clean up the _validateX functions in Route Manager, Route Resource and Event just to name a few. Overall to me, it makes sense for the adonis core to use static typing.

Introducing Typing

I know @thetutlage said he'd started some work with typescript, but I do think flow is worth a look. There's actually 2 considerations here: the core and then the availability as flow-typed modules. Is there a strong opinion against transpiling the core? If not, I think flow/babel should definitely be considered alongside typescript for typing.

As for the app itself, the main use case for flow (at least in my case) is complex apps that integrate flow in the frontend (react in my case, which plays nicer with flow than typescript imo & is used more among react projects than typescript), being able to share type definitions between adonis and react is extremely helpful. There are some other benefits to flow (e.g. existential, type-spread, and the implicit type inference is nice, but this is the biggest one and the reason I have my project setup in the adonis-flow-example schema. The biggest issue I'm running into from the app side is the same issue that the typescipt implementation is running into, creating a custom resolver around the use keyword.

It looks like facebook/flow#6132 added the ability to use custom module resolvers, but as of the latest changelog it's still experimental. I've opened up a PR facebook/flow#7014 explaining our use case and the custom-resolver requirement. I definitely think it makes sense to tackle flow/typescript together since the only difference is going to be the types as far as I can tell. If typescript support for apps is in 5.x, I'd say flow + typescript support is definitely worth looking into.

Either way, I definitely think more typescript in the core is a good thing.

jakesylvestre commented 6 years ago

Ongoing discussion on facebook/flow#7014. It looks like this is possible with their custom-resolver module. Basically, we'd create "a long-running binary that Flow starts and communicates with using a line-by-line stdin/stdout formatted with JSON."

The current example they have is from the yarn codebase. It basically involves listening to that buffer and getting the module by its path and creating a map of strings to their files. This is then written to a file (in Yarn's case, the Yarn.lock file). There's also a pretty good example in the form of a test which just resolves paths.

As @arcanis explained: "it just tells Flow which file to load when file X makes a require(Y) call". It looks like this whole thing can pretty easily be run from .flowconfig meaning we could probably substitute out the resolver here for something derived from a resolver also being used for typescript.

If we went with typescript support, it seems like the LoE to add typedefs would be pretty low once the resolver was implemented with packages like https://github.com/joarwilk/flowgen

thetutlage commented 6 years ago

Hello @jakesyl ๐Ÿ‘‹ Thanks for creating such a detailed issue :)

Yes, I am re-writing everything in Typescript, which means that you can get benefits of strict types and intellisense at the same time.

As far as flow is concerned, I am not sure if flow can make use of Typescript type definitions or can they can converted to flow types without any extra manual effort.

I personally don't have enough bandwidth to do extra work to support flow.

jakesylvestre commented 6 years ago

@thetutlage, I might actually take a look at submitting a PR around flow support if that's something you'd be interested in. I'd definitely utilize the typescript definitions to generate the flow definitions

thetutlage commented 6 years ago

@jakesyl Yeah that will be nice. However, I suggest holding for a while, I am working on this two rfcs. https://github.com/adonisjs/rfcs/issues/1 and https://github.com/adonisjs/rfcs/issues/2.

As soon as I am done, you can feel free to submit a PR. Or if it's just about setting up the tooling, then you can still create a PR against the next branch

jakesylvestre commented 6 years ago

@thetutlage looking forward to it. Anything I can do to help out on TS?

thetutlage commented 6 years ago

@jakesyl Yeah sure. I am not sure how much expertise you got in Typescript. Me and @RomainLanz are pretty new to TS and liking it so far.

Since you know that AdonisJs has a pluggable nature, where new packages can extend the core functionality of the framework, for example: @adonisjs/auth adds auth object to the HTTPContext.

In case of Typescript, we also have to look into a way of merging the type declarations of these packages, when they extend or attach new properties to existing objects. Me and @RomainLanz have been trying out different ways to get it working with minimum efforts, however, we are happy to have more opinions on same.

If you fancy spending time on same, then I can give the full download and discuss more about it.

jakesylvestre commented 6 years ago

@thetutlage yeah that sounds good. I'll do some research today and see what I come up with. Would love to hear where you guys have gotten so far

iam4x commented 6 years ago

Hey ๐Ÿ‘‹

I'm a big fan of flow but I had really hard times with use() typing, that's how we achieved it finally:

Then we overloaded the use() method that way:

This is working great, VSCode auto-completion and type-checking BUT:

I'm currently working on a POC of a custom use() function that could fixes theses issues.

Otherwise, since Babel7 has TS support I would be happy to give TS another try ๐Ÿ‘ (Bad experience with AngularJS)

iam4x commented 6 years ago

Oh and for instance, that's how we type a model as well:

declare class api$InvoiceModel extends adonis$Model<api$InvoiceModel> {
  reference: string;
  invoice_type: string;
  refunding_invoice_id: string;
  order_id: number;
  charge_at: ?Date;
  paid_at: ?Date;

  static types: { [key: string]: string };

  static generate(
    order: api$OrderModel | Object,
    orderItems: api$OrderItemModel[],
    invoiceType: string,
    parentInvoice?: number
  ): Promise<api$InvoiceModel>;

  order(): api$OrderModel;
  capturedPayments(): api$PaymentModel;
  payments(): api$PaymentModel;
  orderItems(): api$OrderItemModel;

  amountDue(): Promise<number>;
  amount(): Promise<number>;
  refundableAmount(): Promise<number>;
  isPaid(): Promise<boolean>;
  isUnpaid(): Promise<boolean>;
}
thetutlage commented 6 years ago

We are already moving to Typescript, so handful of issues will get fixed.

RFC Progress

jakesylvestre commented 4 years ago

Alright, since the entire repo's been reimplemented in typescript as per adonisjs/rfcs#1, I'm going to close this issue. I hope this was helpful in making a decision/the flow examples were useful!