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: add CancelToken and isCancel to axios instance #292

Merged
merged 6 commits into from Oct 23, 2019
Merged

Conversation

3b3ziz
Copy link
Contributor

@3b3ziz 3b3ziz commented Oct 9, 2019

Providing a solution for #271 by adding CancelToken and isCancel to the exported axios instance.

Reference: axios/axios#1330 (comment)

@codecov
Copy link

@codecov codecov bot commented Oct 9, 2019

Codecov Report

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

Impacted file tree graph

@@         Coverage Diff         @@
##            dev   #292   +/-   ##
===================================
  Coverage   100%   100%           
===================================
  Files         1      1           
  Lines        28     28           
  Branches     13     13           
===================================
  Hits         28     28

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 fdb9837...03a7b20. Read the comment docs.

@SaulIO
Copy link

@SaulIO SaulIO commented Oct 18, 2019

Please make this happen.

@pi0
Copy link
Member

@pi0 pi0 commented Oct 21, 2019

The plugin (generated) by axios module (@nuxt/axios) is inside .nuxt directory. So at least it is not a solution for #271 (to import from '@nuxtjs/axios'). Making this shortcut could be nice but I appreciate it if you can also add new usage to the docs. (and mentioning why it makes it easier)

@Amrmak
Copy link

@Amrmak Amrmak commented Oct 21, 2019

@pi0 would that help ? it's quoted from axios docs

SaulIO
Copy link

@SaulIO SaulIO commented on 74daad4 Oct 22, 2019

Choose a reason for hiding this comment

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

Add a link to that doc anchor too, please.

docs/usage.md Outdated

```js
const CancelToken = this.$axios.CancelToken;
const source = CancelToken.source();
Copy link
Member

@pi0 pi0 Oct 22, 2019

Choose a reason for hiding this comment

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

As we just use it once:

Suggested change
const source = CancelToken.source();
const source = thus.$axios.CancelToken.source();

docs/usage.md Outdated

this.$axios.$get('/user/12345', {
cancelToken: source.token
}).catch(function (thrown) {
Copy link
Member

@pi0 pi0 Oct 22, 2019

Choose a reason for hiding this comment

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

Suggested change
}).catch(function (thrown) {
}).catch((error) => {

docs/usage.md Outdated
cancelToken: source.token
}).catch(function (thrown) {
if (this.$axios.isCancel(thrown)) {
console.log('Request canceled', thrown.message);
Copy link
Member

@pi0 pi0 Oct 22, 2019

Choose a reason for hiding this comment

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

Suggested change
console.log('Request canceled', thrown.message);
console.log('Request canceled', error.message);

docs/usage.md Outdated
You can create a cancel token using the `CancelToken.source` factory as shown below:

```js
const CancelToken = this.$axios.CancelToken;
Copy link
Member

@pi0 pi0 Oct 22, 2019

Choose a reason for hiding this comment

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

Suggested change
const CancelToken = this.$axios.CancelToken;

docs/usage.md Outdated
cancelToken: source.token
}).catch(function (thrown) {
if (this.$axios.isCancel(thrown)) {
console.log('Request canceled', thrown.message);
Copy link
Member

@pi0 pi0 Oct 22, 2019

Choose a reason for hiding this comment

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

Suggested change
console.log('Request canceled', thrown.message);
console.error('Request canceled:', error);

docs/usage.md Outdated
You can also create a cancel token by passing an executor function to the `CancelToken` constructor:

```js
const CancelToken = this.$axios.CancelToken;
Copy link
Member

@pi0 pi0 Oct 22, 2019

Choose a reason for hiding this comment

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

Suggested change
const CancelToken = this.$axios.CancelToken;
const { CancelToken } = this.$axios;

docs/usage.md Outdated
let cancel;

this.$axios.$get('/user/12345', {
cancelToken: new CancelToken(function executor(c) {
Copy link
Member

@pi0 pi0 Oct 22, 2019

Choose a reason for hiding this comment

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

Can't we simply instantiate CancelToken outside?

Copy link

@SaulIO SaulIO Oct 22, 2019

Choose a reason for hiding this comment

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

These are the examples in the axios docs.

Personally I'd leave it as is and state that you're quoting rather than treating these doc as part of this project.

Copy link
Member

@pi0 pi0 Oct 22, 2019

Choose a reason for hiding this comment

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

OK :) But i would suggest update function to arrow

@pi0
Copy link
Member

@pi0 pi0 commented Oct 22, 2019

Thanks @Amrmak . Just left some minor comments.

@Amrmak
Copy link

@Amrmak Amrmak commented Oct 22, 2019

Thanks @pi0 for the feedback.
Kindly check the applied comments and if you have further comments.

SaulIO
Copy link

@SaulIO SaulIO commented on e84f0af Oct 23, 2019

Choose a reason for hiding this comment

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

They might be using a named function for a reason: eg, see the issue re named functions in event handlers.

I know executor is on some TS interfaces.

Copy link
Member

@ricardogobbosouza ricardogobbosouza left a comment

@Amrmak minor comments

Please remove all ;

docs/usage.md Outdated
You can create a cancel token using the `CancelToken.source` factory as shown below:

```js
const { CancelToken } = this.$axios;
Copy link
Member

@ricardogobbosouza ricardogobbosouza Oct 23, 2019

Choose a reason for hiding this comment

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

No need this line

docs/usage.md Outdated
} else {
// handle error
}
});
Copy link
Member

@ricardogobbosouza ricardogobbosouza Oct 23, 2019

Choose a reason for hiding this comment

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

Formatting:

this.$axios.$get('/user/12345', {
  cancelToken: source.token
}).catch(error => {
  if (this.$axios.isCancel(error)) {
    console.log('Request canceled', error)
  } else {
    // handle error
  }
})

docs/usage.md Outdated
{
cancelToken: source.token,
},
);
Copy link
Member

@ricardogobbosouza ricardogobbosouza Oct 23, 2019

Choose a reason for hiding this comment

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

Formatting

this.$axios.$post('/user/12345', {
  name: 'new name'
}, {
  cancelToken: source.token 
})

@SaulIO
Copy link

@SaulIO SaulIO commented Oct 23, 2019

@Amrmak minor comments

Please remove all ;

Please don't. The no- semi-colon movement is going to make it impossible to optimise JITs someday soon.

Python is a whitespace-organised language. JavaScript isn't and it's going to cause trouble trying to make it one.

@pi0
Copy link
Member

@pi0 pi0 commented Oct 23, 2019

@SaulIO We are following standardJS with eslint-config and also docs and examples so it is appreciated to keep it consistent. But of course, it is 100% optional.

PS: I promise no perf advantages are there when code is compiled/transpiled :)

@SaulIO
Copy link

@SaulIO SaulIO commented Oct 23, 2019

@SaulIO We are following standardJS with eslint-config and also docs and examples so it is appreciated to keep it consistent. But of course, it is 100% optional.

PS: I promise no perf advantages are there when code is compiled/transpiled :)

Sad to see the StandardJS movement adding another victim. :P

As I understand it, ambiguity in syntax makes JITing harder and consistency makes it easier, and global consistencies allow global optimisations: See discussion a here re Dart's choices: dart-lang/sdk#30347 (comment)

But OK, I didn't realise it was project standards, even if I wish you'd gone the other way.

@pi0
Copy link
Member

@pi0 pi0 commented Oct 23, 2019

(off-topic) @SaulIO JIT executes the final bundle. No matters how we write original code (with or without semi), webpack, babel and terser transform it to the same output :)

@SaulIO
Copy link

@SaulIO SaulIO commented Oct 23, 2019

(off-topic) @SaulIO JIT executes the final bundle. No matters how we write original code (with or without semi), webpack, babel and terser transform it to the same output :)

(continuing off-topic) Right, but shouldn't we have a "run anywhere" standard for portability?

Ie, we'd want it to behave the same in tooled vs untooled environments.

docs/usage.md Outdated
this.$axios.$post('/user/12345', {
name: 'new name'
}, {
cancelToken: source.token
Copy link
Member

@ricardogobbosouza ricardogobbosouza Oct 23, 2019

Choose a reason for hiding this comment

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

Fix indentation

lib/plugin.js Outdated
@@ -186,6 +186,8 @@ export default (ctx, inject) => {

// Create new axios instance
const axios = Axios.create(axiosOptions)
axios.CancelToken = Axios.CancelToken;
axios.isCancel = Axios.isCancel;
Copy link
Member

@ricardogobbosouza ricardogobbosouza Oct 23, 2019

Choose a reason for hiding this comment

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

Remove ;

pi0
pi0 approved these changes Oct 23, 2019
Copy link
Member

@pi0 pi0 left a comment

Awesome work ❤️

@pi0 pi0 changed the title feat: add CancelToken and isCancel to axios instance feat: add CancelToken and isCancel to axios instance Oct 23, 2019
@pi0 pi0 merged commit b9335b1 into nuxt-community:dev Oct 23, 2019
3 checks passed
@pi0
Copy link
Member

@pi0 pi0 commented Oct 23, 2019

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.

None yet

5 participants