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

feat: typescript typings and example #486

Closed
wants to merge 14 commits into from

Conversation

NickBolles
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Nov 20, 2019

Codecov Report

Merging #486 into dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##               dev      #486   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines            2         2           
=========================================
  Hits             2         2           

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 b9d0555...488595a. Read the comment docs.

@NickBolles
Copy link
Contributor Author

Followup - Remove package from DefinitelyTyped - https://github.com/DefinitelyTyped/DefinitelyTyped#removing-a-package

@NickBolles
Copy link
Contributor Author

reminder to look at changes to the @types package #40778 linked above and DefinitelyTyped/DefinitelyTyped#40840 before merging

@davidlandais
Copy link

Why are you waiting to merge this PR ? 👍

Copy link
Member

@pi0 pi0 left a comment

Choose a reason for hiding this comment

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

Thanks for PR

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@pi0 pi0 requested a review from kevinmarrec January 13, 2020 10:19
@pi0 pi0 changed the title Typescript - Bundle typings and add setup docs feat: typescript typings and example Jan 13, 2020
docs/guide/setup.md Outdated Show resolved Hide resolved
docs/guide/setup.md Outdated Show resolved Hide resolved
examples/with-typescript/mixins/auth.ts Outdated Show resolved Hide resolved
examples/with-typescript/mixins/auth.ts Outdated Show resolved Hide resolved
examples/with-typescript/nuxt.config.ts Outdated Show resolved Hide resolved
examples/with-typescript/package.json Show resolved Hide resolved
examples/with-typescript/tsconfig.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
types/index.d.ts Outdated Show resolved Hide resolved
@NickBolles
Copy link
Contributor Author

Thanks for the review @kevinmarrec and @pi0 I'll look at these today, rebase and resubmit.

@NickBolles
Copy link
Contributor Author

As part of the conversion to use async as requested. I removed the comments suggesting async


    // If you are not fond of using axios promises on async calls
    // You can still use Javascript try and catch block
    //
    // try {
    //   this.$toast.show('Logging out...')
    //   await this.$auth.logout()
    //   this.$toast.success('Successfully disconnected')
    // } catch (err) {
    //   this.$toast.error('Error while connecting: ' + err.message)
    // }

@NickBolles
Copy link
Contributor Author

NickBolles commented Feb 4, 2020

@kevinmarrec @pi0

Thanks for the review @kevinmarrec and @pi0 I'll look at these today, rebase and resubmit.

So much for "today"... All of these should be fixed finally. The examples should be working correctly too. Let me know if you see anything else!

@localhost5001
Copy link

Any updates?

Copy link
Member

@pi0 pi0 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR ❤️ I put some comments.

lib/core/middleware.js Show resolved Hide resolved
lib/core/hook.js Outdated Show resolved Hide resolved
@NickBolles
Copy link
Contributor Author

@pi0 @kevinmarrec Could you guys take another look? somehow I merged two branches and added a bunch of incorrect changes in the middle of this PR. (probably cuz I'm on a dev branch and not a feature branch....) Should be good to go.

Should also make changes like this: DefinitelyTyped/DefinitelyTyped#43380 easier

Copy link
Member

@kevinmarrec kevinmarrec left a comment

Choose a reason for hiding this comment

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

Sounds good to me

@NickBolles NickBolles requested a review from pi0 April 6, 2020 17:55
Copy link
Collaborator

@JoaoPedroAS51 JoaoPedroAS51 left a comment

Choose a reason for hiding this comment

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

Hi! Nice work! 😃 I left some comments related to latest changes on dev branch.

types/index.d.ts Outdated Show resolved Hide resolved
types/index.d.ts Outdated Show resolved Hide resolved
types/index.d.ts Outdated Show resolved Hide resolved
examples/with-typescript/pages/secure.vue Outdated Show resolved Hide resolved
types/index.d.ts Show resolved Hide resolved
@NickBolles
Copy link
Contributor Author

NickBolles commented Apr 9, 2020

Alright I think I've made those updates besides RefreshController and RequestHandler.

Side note: am I supposed to mark the comments as resolved? or leave that for reviewers to resolve?

Also, I'm really tempted to just rename everything to .ts and add a build step. It would make these types way more complete and way less maintenance, not to mention make the library more approachable for newcomers, since typescript is easier to see mistakes.

@t-eckert
Copy link

As a user, looking forward to seeing this merged!

Copy link
Collaborator

@JoaoPedroAS51 JoaoPedroAS51 left a comment

Choose a reason for hiding this comment

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

Hi! Nice work! Almost done! 😄 I think there is only one last thing to fix.

@@ -0,0 +1,8 @@
export default class RequestHandler {
constructor(auth: any);
$auth: any;
Copy link
Collaborator

Choose a reason for hiding this comment

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

$auth shouldn't be the Auth class?

tpscrpt added a commit to tpscrpt/auth-module that referenced this pull request Apr 18, 2020
tpscrpt added a commit to tpscrpt/auth-module that referenced this pull request Apr 18, 2020
tpscrpt added a commit to tpscrpt/auth-module that referenced this pull request Apr 18, 2020
Also, import @nuxt/auth relatively in the typescript example's package.json
@NickBolles
Copy link
Contributor Author

closing in favor of #622

@NickBolles NickBolles closed this Apr 19, 2020
@pi0
Copy link
Member

pi0 commented Apr 21, 2020

Hi @NickBolles Nice work. I may bring some parts from your works but it will only cover internal TS rewrite and auto-generated types.

Are you interested in helping with a new typescript section in docs and creating a repository for with-typescript example? We can then link it from docs :)

@NickBolles
Copy link
Contributor Author

Sure! Where/when is the typescript re-writes happening? I was tempted to just rename everything to .ts and add a compile step instead of adding the types.

I'd be interested in helping out or reviewing the typescript migration.

@JoaoPedroAS51
Copy link
Collaborator

@NickBolles Hi! Check #621

@NickBolles
Copy link
Contributor Author

@NickBolles Hi! Check #621

Perfect. Thanks!

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.

7 participants