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

Rough idea for what a comparison should cover #1

Closed
paulcsmith opened this issue Jun 25, 2018 · 59 comments
Closed

Rough idea for what a comparison should cover #1

paulcsmith opened this issue Jun 25, 2018 · 59 comments

Comments

@paulcsmith
Copy link

paulcsmith commented Jun 25, 2018

IMO Every bullet point should have 1) what it looks like (real code) 2) pros and cons of the approach 3) how the design prevents bugs 4) any other considerations framework authors think are important

  • How do you define routes and actions (include accessing params from within actions as well)
  • How do you call endpoints from within HTML
  • How do you query data
  • How do you handle complex queries (not, greater than, joins)
  • How do you validate params/data
  • How do you save data
  • How do you write HTML
  • How do you write HTML forms
  • How do you write data migrations
  • How do you define a model
  • How is the project structured

Framework priorities (main goals)

What does each of the frameworks prioritize? Type-safety? Speed? Familiarity?

Ease of learning

  • How are the guides?
  • How many ways are there to do things?
  • How familiar is to existing solutions, and what are those solutions?
  • Anything other parts that are easy/hard to learn?

Performance

I'd suggest that we give rough benchmarks and say that all the Crystal frameworks are fast and the differences are negligible. They're all so close that I think performance should probably not even be a concern when choosing. Thoughts?

@paulcsmith
Copy link
Author

@paulcsmith
Copy link
Author

My thought is that we can put ideas in this issue for things we want to cover.

Once we have that outline set, both teams can create a markdown doc in this repo that is based on the template above.

We can then make a PR for them so each team can comment with questions/concerns, etc.

Once we feel good about it where the docs are at, we can come up with a master document that combines them both.

@waghanza
Copy link

I'd suggest to talk also about performance depending on use case. I mean :

Why should I priorize type safety (example) ?

If for example designing a RESTful API

@drujensen
Copy link

Couple more items:

  • What is your main goal?
  • What design patterns do you embrace?
  • How is your project structured?

@paulcsmith
Copy link
Author

That's a great point. Maybe we should also address those aspects. Why would you want type-safety? What kind of bugs will it catch? What kind of projects does it matter for?

@paulcsmith
Copy link
Author

@drujensen I like the first 2. I'm not sure about "How is your project structured?" unless there are a list of reasons to go with it. And a list of reasons for the design patterns too. For example, I'd love to know why certain design patterns were chosen

@drujensen
Copy link

I think that is more of a language comparison, right? As long as you don't use run-time evaluations, you will catch them.

@waghanza
Copy link

Sure. First of all, i thing we SHOULD / COULD decide to turn this in a decision-making project

@paulcsmith
Copy link
Author

I think that is more of a language comparison, right? As long as you don't use run-time evaluations, you will catch them.

I'm not sure what you are referring to. Could you clarify what you mean?

@drujensen
Copy link

I'm not sure about "How is your project structured?

We use the same project structure as rails to make the transition easy. Not sure if Lucky does this or not.

@paulcsmith
Copy link
Author

@waghanza I think the comparison is to help people make a decision so I think we're on the same page 👍

We use the same project structure as rails to make the transition easy. Not sure if Lucky does this or not.

Ah I see. Lucky is similar, but not quite the same. I'll add that to the list of things to cover!

@drujensen
Copy link

I'm not sure what you are referring to. Could you clarify what you mean?

Type safety is built into Crystal. Crystal is a strongly typed language. This is not a feature of Amber or Lucky. This is a feature of Crystal.

@paulcsmith
Copy link
Author

paulcsmith commented Jun 25, 2018

@drujensen Ah I see what you mean. It is a feature of Lucky in that it uses the type-system to catch a lot of stuff that I don't think Amber does catch. That is one of the key differences in Lucky and it is one that people often overlook without clear examples. Once I write up the comparison docs for Lucky I think it will show what I mean, but here is a quick example:

class Users::Show < BrowserAction
  # The route macro automatically generates a RESTful route based on the class name
  route do
    # Type safe query param. If `id` does not exist it will fail to compile. There is no accessing query params with symbols or strings
    UserQuery.find(id)
    text user.name 
  end
end

# in a view
# Type-safe routing. If the action doesn't exist it will fail to compile
# If you pass the wrong arguments it will fail to compile
link user.name, to: Users::Show.with(user.id)

# It also automatically sets the HTTP method, so you can never accidentally use the wrong one
# This will automatically make the link use the DELETE HTTP method
link "Delete user", to: Users::Delete.with(user.id)

Lucky tries to leverage the type-system everywhere possible so it can catch as many bugs as possible and help you debug quickly with nice compile time errors.

@drujensen
Copy link

drujensen commented Jun 25, 2018

Users::Show.with(user.id)

I see. We have (or will have) helpers for users_path just like rails to avoid this runtime error. This is just avoiding using DSL that would be interpreted at runtime instead of compile time.

Do you support both options? link "Delete user", url: "/users", method: "delete" or are you saying Lucky is strict in that you are not allowed to use a runtime evaluation?

@paulcsmith
Copy link
Author

Yes, you can also use a string path, but mostly only used for externals links since it is not safe. Using the action helpers is safer, and easier to read so that's what's documented and what most people use

@drujensen
Copy link

drujensen commented Jun 25, 2018

Do you have other examples on Lucky and type safety that you think differentiates it from Amber?

To me, Amber is just as safe as Lucky for avoiding runtime errors that plagues the Interpreted languages like Ruby, Python, Elixir, Node, PHP, Perl, R, etc. But, I may be missing something.

@paulcsmith
Copy link
Author

paulcsmith commented Jun 25, 2018

@drujensen The example above showed a few, but I can share some others here too.

In the example above: Type safe params access

class Projects::Tasks::Index < BrowserAction
  nested_route do
     # Will be caught at compile-time because `id` is not available for this action. It should be `project_id`
     ProjectQuery.find(id)
     # In Amber you'd do this and it would not be caught at compile-time
     params[:id]
  end
end

You can also do this for query params:

class Projects::Tasks::Index < BrowserAction
  param page : Int32 = 1

  nested_route do
     # You can use the `page` variable
     # Lucky will ensure that you use an integer when generating a link, and will automatically cast it to an int for you
     page
  end
end

Querying

No symbols or string column names, association names, or comparison operators.

# If you rename or mistype the 'name' column you'll get a compile-time error
# `lower` is only available for string columns. You'll get a compile-time error if you use it on something else
# The values are type-safe. If you try to pass an `Int32` it will raise at compile time
UserQuery.new.name.lower("paul")

# Complex joins and preloading are also type-safe
# If you mistype or rename `author` you'll get an error at compile-time
UserQuery.new.preload_author

# Will raise if you mistype or rename the comments model or the rating column
UserQuery.new.join_comments.comments(&.ratings.gt(4))

Validations & strong params

class SignUpForm < User::BaseForm
  fillable email
  virtual password : String
  virtual password_confirmation : String

  def prepare
     # Validations are type-safe
     # They will raise if you rename these fields for example
     validate_confirmation_of password, with: password_confirmation
    # In Amber you might validate params like this
    # This will not catch mistyped field names at compile-time
    validate(:my_field) { ... } 
  end
end

Strong params are also type-safe in Lucky

# Since we marked `email` as `fillable` Lucky knows it can be used in a form
f = SignUpForm.new
text_input f.email

# But if I try to use the name field, but don't make it fillable
text_input f.name

# It will raise at compile-time with a friendly error:
The field is not fillable.

Try this...
  ▸ Allow the field to be filled by adding 'fillable {field_name}' to your form object.

Q. Why aren't fields fillable by default?
A. Malicious users could submit any field they want. For example: you
   might have an 'admin' flag on a User. If all fields were fillable,
   a malicious user could set the 'admin' flag to 'true' on any form.

It will also raise if you mistype or rename a column

text_input f.socail_security_number

## Did you mean f.social_security_number?

In HTML

# Most templating languages happily print `nil` as an empty string. This leads to lots of annoying bugs
# So if you have a project that has a description that could be `nil`
# This will print an empty paragraph
# Almost never what you want
<p><%= article.description %></p>

# In Lucky it will raise at compile-time
para article.description

# Something like
No overload for `para text : Nil`

Must be `para text : String`

# You can then do this and it will only print the paragraph tag if there is a value
article.description.try do |value|
  para value
end

In Models

# Because models and validations/saving are split-up, Lucky can be more specific about the types
class User < BaseModel
  table :users do
     column name : String
     column nickname : String?
  end
end

# If I do this, it works, because the method for `name` only returns a `String`
UserQuery.first.name.upcase

# And will raise if I do it for nickname, since it is nilable
UserQuery.first.nicknamename.upcase

# With a lot (maybe all?) the other ORMS, the fields are nilable 
# because you can instantiate an invalid record
user = User.new
user.name # String | Nil

# Which means you have to call `not_nil!` which is error-prone
user.name.not_nil!.uppercase

# But what happens if later you make name optional!?
# This will blow-up at runtime in most ORMs in Crystal
# So you'd have to have a spec to test for it, or remember to check everywhere that calls the method
user.name.not_nil!.upcase

# Whereas Lucky will blow up at compile-time if you make it optional
user.name.upcase # Can't upcase on Nil

# So you could then handle it nicely
user.name.try(&.upcase)

Maybe part of the confusing is that I'm saying Lucky is more type-safe. Maybe it's more clear to say that it prevents more runtime errors.

Preventing runtime errors has driven Lucky's design from the beginning so that you can catch more errors, faster. A lot of these errors can't be caught by the design system with other design patterns. Happy to discuss more, and if I'm wrong about what Amber can do, LMK. I'm fairly certain these examples are correct though and show ways that Lucky can catch things that other libs can't

@drujensen
Copy link

drujensen commented Jun 25, 2018

@paulcsmith Aah, Yes. Thanks for all the examples. I can see some of these areas as being runtime errors in Amber, especially the strong params.

The HTML not so much since our templates are compiled in Amber, but it sounds like you are focused on avoiding these runtime errors, where for Amber, its more of an afterthought and not a priority.

@paulcsmith
Copy link
Author

@drujensen You're welcome. I'm glad that made things more clear :)

The HTML example is still true because the compiled template calls to_s on the object so even nil will compile fine, it will just print nothing. So it won't result in a runtime error, but 90% of the time it's not what you want when writing HTML views. Does that make more sense?

@paulcsmith
Copy link
Author

paulcsmith commented Jun 25, 2018

Or maybe slang uses a different method of compiling that blows up if passed nil. That would be very cool :) I'm fairly certain ECR will print an empty string though :(

@drujensen
Copy link

drujensen commented Jun 25, 2018

Not sure what you mean by to_s, but I am more referring to type safety as in Ruby vs Crystal. If I try to access a variable that is undefined in an slang or erb template, it will fail in Amber.

@paulcsmith
Copy link
Author

paulcsmith commented Jun 25, 2018

Sorry for being so unclear. I mean that this will compile:

class User
  getter nickname = nil
end

# In your template
Nickname: <%= User.new.nickname %>

Prints: Nickname:, which is usually not the desired behavior

But yeah, <%= User.new.nickname.upcase %> would be caught by Lucky or any templating language based on Crystal

@paulcsmith
Copy link
Author

Oh and what I meant by to_s:

https://github.com/crystal-lang/crystal/blob/4547b6b92b9d289890017d9e591d5ef3785a011e/src/ecr/processor.cr#L43

When an ECR template is compiled, it wraps the stuff in <%= %> and calls to_s on it. Which is why nil works. <%= nil %> is translated to (nil).to_s

@drujensen
Copy link

Lol. Didn't realize that. That seems like a bug in the ECR processor. I wonder if slang does the same thing?

Regarding my misunderstanding: I think the terminology of type safety may be where I misunderstood this difference.

I think you are trying to avoid run-time errors. This comes at a cost though. You will have to use crystal code for everything. No SQL, JSON, YAML, XML, HTML, MD, JS or other DSL that is evaluated at runtime will be allowed, right?

@paulcsmith
Copy link
Author

paulcsmith commented Jun 25, 2018

Yeah, it may be a bug. I believe Slang does the same and I think it would be hard to fix because you'd have to run semantic phase of the compiler on the whole program to know if the expression can output nil :S. Or you could not call to_s on it, but that would cause issues because you couldn't print integers and things like that without manually adding to_s (e.g. <%= 1.to_s %>). I'm not sure what the best solution there is.

Yes there is some cost there, and some things simply can't be avoided, but the aim is to avoid as many runtime errors as possible. That is why Lucky avoids YML or JSON for config, in favor of Habitat which raises at compile-time on incorrect types, incorrect settings and raises at startup if you're missing config. It's also why HTML is done in Crystal. You get better control over the types, and you can use regular Crystal methods for extracting methods, classes and modules.

SQL is covered by LuckyRecord for the most part. Parsing JSON mostly does not result in runtime errors as long as it is properly formatted. LuckyRecord handles most of it and returns validation errors, but it could be better

JS is also not really type-safe (with Crystal) at all. You need LuckyFlow for acceptance tests. You could use TypeScript of course, but that still wouldn't ensure that the data you're passing from Lucky -> your JS is in the correct format. Maybe with GraphQL schemas this could be alleviated, but that's pretty far down the pipe

So there is room for improvement for sure, but a lot of cases are covered for you

@eliasjpr
Copy link

eliasjpr commented Jun 25, 2018

Happy to see this collaboration happening.

Paul I see what you mean in regards to the ecr templating engine, but I’m sure if that’s a fair comparison, since Lucky does not use a template engine.

Now it is neat that Licky can catch those at compile time. I think this is a key differentiator between the two frameworks, the fact that Amber uses template engines for view vs Lucky is tightly integrated

@paulcsmith
Copy link
Author

paulcsmith commented Jun 25, 2018

@eliasjpr Me too! Happy to get this ball rolling.

I think it is a fair comparison because they are two different approaches to how views are created and one catches printing nil and the other doesn't. They are different, but they are used for the same purpose. I'm not sure how else to compare them. I think there is definitely more to it than just compile-time, but I was mostly focusing on that for now.

The downsides are that you need to learn something new, you have to convert HTML snippets to Crystal and a few others. I'd love to get into the nitty-gritty once we agree on the main points we'd like to address.

I feel pretty good about what's in the initial post, but I'll wait for the Amber team as you may want to add some more points!

Once we feel good about the list, LMK so that we can start working on the documents that go over in-depth all different ways that Lucky and Amber approach things! :D

@eliasjpr
Copy link

In regards to the configs Yaml in Amber they are type checked and this happens when the yaml gets parsed all the yaml fields gets parsed to their corresponding types and if this does not match it will error, if the developer types the wrong type value for a given yaml field it will error as it will do with Habitat

@paulcsmith
Copy link
Author

@eliasjpr That is cool! I didn't realize it did that 👍

@eliasjpr
Copy link

Yes definitely a fair comparison!

Sorry typing from my phone, I meant to say it is a fair comparison :)

@paulcsmith
Copy link
Author

Though I just tried Amber and it looks like the YAML parsing is using a macro so it does happen at compile-time. Pretty sweet!

@eliasjpr
Copy link

Paul in regards to params Granitw will error out of you pass and invalid type field, since the field definition in granite are type safe.

Does lucky parses the request body/query params to its type when a request comes in?

All request params url/route/body are strong in the request there is virtually no way to unless the params at some point are mapped to their corresponding type, In Amber this is done at the model level when an instance of a model is created.

@waghanza
Copy link

I think will also SHOULD talk about learning curve

@eliasjpr
Copy link

For instance if I access ‘?id=123’ in Lucky action would this be an interger?

@paulcsmith
Copy link
Author

@eliasjpr But does it give you that error at runtime or compile-time? That's the key difference. If it is at compile-time it means you don't need a test to catch that particular bug, it points exactly to where the problem is, and saves you time. At runtime, it is not quite as helpful. I'm fairly certain that if you mistype a key in Amber, it will fail at runtime (for params in controllers):

params.validation do
   # Typo won't be caught until runtime
    required(:socail_security_number, "Must be present!") { |p| ... }
 end

It seems that Granite validations are type-safe since you have a full object and you're not using symbols, which is pretty dang cool. It still has issues making all columns nilable to cover that though

Lucky parses query params when they come in, and the form object parses params according to their type.

@paulcsmith
Copy link
Author

paulcsmith commented Jun 25, 2018

@eliasjpr query params are since you give them a type: param page : Int32 = 1 will be cast to an int and will raise if it you pass ?page=hello

For the id, it will be a string, but LuckyRecord will cast it. In the future though, the route params will also be typed and they won't be ints. They will be specific to a model:

class Projects::Tasks::Show < BrowserAction
  nested_route do
    id # => Returns Task::Id object
    project_id # +> Returns Project::Id object

    # Accidentally using the wrong id
    ProjectQuery.find(id) # Compile time failure: Can't use Task::Id. Expecting Project::Id
  end
end

So type-safety will go a bit further. Just haven't had time for that quite yet

@eliasjpr
Copy link

But at runtime I can pass a different value I can pass ‘?id=hhhj’ and it will fail. And this specific case needs to happen at runtime since the framework does not know the actual value of the id

@paulcsmith
Copy link
Author

Great idea @waghanza. I think that is a bit subjective, but we can try to hash it out. I'll add it to the list

@eliasjpr
Copy link

The last example answers my question and I think that is valueable.

@eliasjpr
Copy link

eliasjpr commented Jun 25, 2018

Something that we have not built into Amber because we have had discussions about creating url helpers that will parse params values to its corresponding type

@waghanza
Copy link

sure @paulcsmith learning curve is quite subjective (depending on your background ...), but very valuable if we aim to encourage crystal and lucky / amber adoption

@paulcsmith
Copy link
Author

Oh nice! I think this is valuable discussion @eliasjpr , but it is hard to track in this way. What do you think abut nailing down the things we want to compare, and then further discussing in a PR? That way we can comment on specific code blocks, sentences, etc. And track all the changes more easily.

When the Amber team feels comfortable with what's at the top, LMK and then we can get started on our documents and start hashing things out in detail

@eliasjpr
Copy link

What is interesting is that anywhere you want to use ProjectQuery.find(id) you must use it with a Project::Id object, you cannot use it with an int.

So I wonder what is the overhead there since I have to parse an int to Projecg::Id?

@eliasjpr
Copy link

Lucky parses query params when they come in, and the form object parses params according to their type.

That’s pretty neat I like that

@paulcsmith
Copy link
Author

paulcsmith commented Jun 25, 2018

In terms of performance, it'll be a struct that wraps an Int. It may be a slight hit, but it is worth it for having super robust and refactorable code (IMO). There will also be ways to easily cast it if you have an int: ProjectQuery.find(Project::Id.parse(1))

Does that answer your question?

@drujensen
Copy link

drujensen commented Jun 25, 2018

Back to your ECR example:

class User
  getter nickname = nil
end

# In your template
<%= User.new.nickname %>

Your example will not compile because the type needs to be specified. If you specify a union of (String|Nil):

class User
  getter nickname : String? = nil
end

You should be ok with nil spitting out nothing in the template. I think this is acceptable behavior. You can always remove the Nil from the union to avoid it.

This is still being type checked. Nil allows you to call .to_s on it. The method exists. Now you may think that Nil shouldn't have a to_s, but that was a decision by the Crystal team and not a type checking issue.

@paulcsmith
Copy link
Author

paulcsmith commented Jun 25, 2018

I don't think it is a type-checking issue, but it does result in bugs. And yeah I didn't test that example so that makes sense that you need to specify a union type.

Regardles, I would much rather always know if something is nil and decide how to handle it so I don't get random empty html elements. I'd say 90% of the time I want to either not print the HTML around the nil or I want to display something else like "No nickname". Having the compiler tell me it is nil and force me to handle it makes it much easier to avoid those bugs before they ever make it to production.

So, I don't think is not type-safe, I just think it can lead to unintended and unexpected behavior. That's the kind of stuff I want to avoid.

So I'm not saying ECR is wrong, I'm just saying that for the type of apps I build, I always want to know if I am about to print nil and handle it myself. That way I have the opportunity to prevent bugs before they're shipped to production. This gives me confidence to make changes and add features without worrying "is this thing nil?"

@paulcsmith
Copy link
Author

paulcsmith commented Jun 25, 2018

A specific example:

  • I'm told to add a nickname to the user's profile page
  • I didn't initially write the user model so I'm not aware the nickname is optional and so I get to work!
  • I open the page and add this:
<p><strong>Nickname: </strong><%= current_user.nickname %></p>
  • I add a test case
it "displays the nickname" do
  create_user(nickname: "Bobby")
  get_the_page
  page.should have_content("Bobby")
end
  • Tests pass, looks good in the browser, ship it!

The next day someone notices that if you don't have a nickname it says: "Nickname: ". This is not wrong, but it's also not right. Since ECR and other templating languages print nil I never thought about the case when it didn't exist. So someone has to create a ticket and I have to go back and add a conditional for when the user's nickname isn't there.

If ECR/whatever tells me it might be nil and disallows printing it, I could have caught this before it ever made it to production and saved someone from reporting a bug, prioritizing it, opening up the code again, adding a test case, etc.

I didn't want to beat a dead horse, but just wanted to describe why Lucky doesn't let you print nil. It allows you make changes with confidence and catch bugs earlier in the process IMO

@drujensen
Copy link

I always want to know if I am about to print nil and handle it myself. That way I have the opportunity to prevent bugs before they're shipped to production

I agree with that. It's always preferable to catch these at compile time. Things that can't, you should always write specs around.

https://github.com/crystal-lang/crystal/blob/4547b6b92b9d289890017d9e591d5ef3785a011e/src/nil.cr#L76

I am interested in why this method was added to Nil. I can see this being problematic not only in these templates but elsewhere. Should the Nil class have methods like this or should it fail to compile when you try to call any method on it?

@paulcsmith
Copy link
Author

Personally, I'd prefer it if to_s was removed from nil. I think you're right that it can cause bugs elsewhere. For example, even with Lucky if you use string interpolation it won't catch nil :( `para "Nickname: #{user.nickname}". So I avoid string interpolation in Lucky. I'd much rather it fail at compile time

It's not hard to do user.nickname || "" if I really want an empty string when it is nil.

Removing to_s might break a lot of stuff, but I'd love it if it were removed :). I'd be happy to update my libs

@paulcsmith
Copy link
Author

paulcsmith commented Jun 25, 2018

I'd also much rather have array and hash accessors return nil by default and call []! for the non-nil version. I've had a number of runtime errors because I used first thinking it could return nil like in Ruby

Update: I'm also obsessed with catching things at compile-time, so this may be just me that wants this :)

@paulcsmith
Copy link
Author

@eliasjpr @drujensen How do you feel about the list? Does that look like a good place to get started?

@drujensen
Copy link

@paulcsmith yes, looks good. We can always add more areas if we deem them as important.

@paulcsmith
Copy link
Author

Great! I'll get started on a doc for Lucky and make a PR

@DestyNova
Copy link

DestyNova commented Aug 9, 2018

When building web interfaces with Elixir and Phoenix, I didn't use any of the view generation or forms. The UIs were either in Elm or VueJS and called endpoints that returned JSON.

So I'd be interested to learn more about how one would approach building a single-page app (or rich client app, or whatever they're called now) with Lucky and Amber, and whether doing so negates a lot of the type-safety guarantees and conveniences that are provided with form helpers etc.

@paulcsmith
Copy link
Author

Adding this from
luckyframework/lucky#508 (comment)

In addition to points listed by @paulcsmith I would like to add:

edit/recompile mechanism and speed for seeing changes
different use cases, or developer backgrounds which may better suit one or the other

@paulcsmith
Copy link
Author

@DestyNova integration with front end frameworks and how that impacts type-safe guarantees would definitely be interesting. I'll address that when I have time! The short answer is that it's still helpful for type-safe config, models, params and routing, but you do lose the whole connection from view to action. You can pass the wrong type or number of params to APIs, accidentally submit mistyped or incorrect values for JSON requests, etc.

So you'd still want some end-to-end tests with LuckyFlow or something so that you know everything's hooked up correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants