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

Tests & eslint #4

Merged
merged 4 commits into from
Jul 14, 2015
Merged

Tests & eslint #4

merged 4 commits into from
Jul 14, 2015

Conversation

bryanrsmith
Copy link
Contributor

Basic tests and eslint gulp task that I didn't get in before release.

@EisenbergEffect
Copy link
Contributor

@cmichaelgraham Can you take a look at the TS issues that Bryan mentions above?

@EisenbergEffect
Copy link
Contributor

@bryanrsmith How is the lint setup working for you? If it's working out well, perhaps we should update our other repos. Let me know what you think.

  • For the config issue, you can create an interface and then reference that in your config function. The babel compiler will filter out interfaces but they will still get picked up in the d.ts that is generated.
  • For the IInterceptor interface, I think the definition should be as follows:
export interface IInterceptor {
    request?(request: Request):  Request|Response|Promise<Request|Response>;
    requestError?(error: any): Request|Response|Promise<Request|Response>;
    response?(response: Response): Response|Promise<Response>;
    responseError?(error: any): Response|Promise<Response>;
}

However, it may be that there's an issue in the d.ts generator. @cmichaelgraham Can you take a look at this?

  • Regarding the objects with string key/values, I'm not sure about that. It would work with a Map object, but that's not actually correct in this case. We may just need to go with using Object and provide a bit of documentation.

@bryanrsmith
Copy link
Contributor Author

@EisenbergEffect The lint setup is working fairly well, but I just realized I hadn't checked it with some of the TS changes. It doesn't seem to like the interfaces. I will look at it further tonight, and probably pull out the TS stuff into a separate PR so we can merge the rest of the changes.

I think we could start rolling it out in other repos if you're happy with the rules (I just stole Airbnb's config, removed the react bits, and adjusted one or two rules that didn't work for us). Since we already have failing lint tasks in all the repos, we can probably just swap it out & then get everyone to contribute to cleanup until everything passes.

For the config issue, you can create an interface and then reference that in your config function

I don't think I can use an interface because it's a union type. I think what I want is type aliases, but I can't get that to work with the generator. /cc @cmichaelgraham

export type ConfigOrCallback = IRequestInit|(config: HttpClientConfiguration) => void|string;

...

configure(config: ConfigOrCallback): void {
}

It compiles, but the ConfigOrCallback type definition is missing from the output.

For the IInterceptor interface, I think the definition should be as follows:

Me too, but the generator rejects that syntax.

We may just need to go with using Object and provide a bit of documentation.

Sounds fine to me. Headers|{ [key:string]: string; } does seem to be valid TS, though, so maybe we should look into that with the generator.

@bryanrsmith
Copy link
Contributor Author

@EisenbergEffect @cmichaelgraham I moved the type annotation changes to #5. This one should be good to go.

@EisenbergEffect
Copy link
Contributor

I'll merge momentarily.

@EisenbergEffect EisenbergEffect merged commit 3cdd3d6 into aurelia:master Jul 14, 2015
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

Successfully merging this pull request may close these issues.

2 participants