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

Refactor/client types #2025

Merged
merged 4 commits into from
Jul 5, 2016
Merged

Refactor/client types #2025

merged 4 commits into from
Jul 5, 2016

Conversation

Awk34
Copy link
Member

@Awk34 Awk34 commented Jun 28, 2016

No description provided.

@Awk34 Awk34 added this to the 4.0.0 milestone Jun 28, 2016
@Awk34 Awk34 force-pushed the refactor/client-types branch from 7d42ed5 to f12cad7 Compare June 28, 2016 17:55
@Awk34
Copy link
Member Author

Awk34 commented Jun 29, 2016

@Koslun these changes should fix the TS errors and work in both TS/Flow

@Koslun
Copy link
Member

Koslun commented Jun 30, 2016

@Awk34 Take it we're skipping more specific typings from definition files that would only apply to TS to avoid having a lot of ejs if's and thus more maintainable templates? Especially since many will be useless with Angular 2 with more maintainable templates also being a plus when we're upgrading.

@Awk34
Copy link
Member Author

Awk34 commented Jun 30, 2016

@Koslun I'm just trying to fix all the TS errors that people are seeing when building the app for the first time. I also want to try to keep any type definitions able to be used with both Flow & TS. Is that reasonable?

Also, is this good to merge?

@Koslun
Copy link
Member

Koslun commented Jun 30, 2016

@Koslun I'm just trying to fix all the TS errors that people are seeing when building the app for the first time. I also want to try to keep any type definitions able to be used with both Flow & TS. Is that reasonable?

As I said. Appreciate that restricting type definitions to what works in both flow and TS enables us to avoid having either an extra template file or just a lot of if-statements for when it's TS. Also realize that most of TS-specific logic will be useless with Angular 1 anyway. With Angular 2 using external type definitions will also be needed to a much lesser extent as the Angular 2 libraries are already written in TS. So figure that fully typed TS and Flow code will be a lot more similar then.

So based on that premise I think it's fully reasonable and like the strategy.

If anything I'm more skeptical of Flow itself as it does not currently support definition files and I know no libraries written with Flow, would think React is but cannot find anything to indicate that. Can however somewhat understand wanting to rather work with Babel and thus using Flow with that. And as it's already implemented in this generator I can see the benefits of it.

Also, is this good to merge?

Pretty much, seems to work on my end. Might want to fix two final errors:

[default] project/client/components/auth/auth.service.ts:137:14
    Supplied parameters do not match any signature of call target.
[default] project/client/components/auth/auth.service.ts:162:14
    Supplied parameters do not match any signature of call target.

Looking into it now.

@Awk34
Copy link
Member Author

Awk34 commented Jun 30, 2016

would think React is

Yeah, React uses Flow.

@Koslun
Copy link
Member

Koslun commented Jun 30, 2016

What I don't then get is why they don't clearly state that here: https://facebook.github.io/react/docs/language-tooling.html#flow , or even here: https://flowtype.org/docs/react.html#_. Feels like a pretty meaningful feature and a strong insentive to use Flow when you develop React applications. Not mentioned on React's wikipedia page either.

@Awk34
Copy link
Member Author

Awk34 commented Jun 30, 2016

@Koslun ¯_(ツ)_/¯

They have TypeScript typings, as well as test against TypeScript, CoffeeScript

@Koslun
Copy link
Member

Koslun commented Jul 1, 2016

Shouldn't have changed any behavior just more explicitly sending in undefined, could also just send the functions in the then blocks too but opted to just add small note to documentation and change nothing really.

@@ -100,7 +100,7 @@ export function AuthService($location, $http, $cookies, $q, appConfig, Util, Use
/**
* Gets all available info on a user
*
* @param {Function} [callback] - funciton(user)
* @param {Function} [callback] - optional, function(user)
Copy link
Member Author

Choose a reason for hiding this comment

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

"optional, " is redundant here

@Koslun
Copy link
Member

Koslun commented Jul 1, 2016

Think the fail is unrelated, tried scaling back the changes and getting the same errors when running the tests locally.

@Awk34
Copy link
Member Author

Awk34 commented Jul 1, 2016

Sequelize likes to 💩 itself quite often

@Awk34 Awk34 merged commit 44f1a39 into canary Jul 5, 2016
@dylannnn dylannnn mentioned this pull request Jul 5, 2016
1 task
@Awk34 Awk34 deleted the refactor/client-types branch August 4, 2016 17:13
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.

2 participants