feat: create axios instance on req.$axios for server-middleware use #428

rchl · 2020-10-14T20:16:34Z

An extra instance of axios is created and attached to "req.$axios" so
that server middlewares have access to the axios instance that has
extensions provided by this module.

The "proxyHeaders" option is ignored for that instance as it would be
both awkward to implement and potentially wrong. This is because
"proxyHeaders" is proxying requests from the request that initiated the
page request while server-middleware instance is kinda independent of
individual page requests.

Resolves #427

codecov · 2020-10-14T20:17:45Z

Codecov Report

Merging #428 into master will decrease coverage by 37.69%.
The diff coverage is 42.98%.

@@             Coverage Diff             @@
##           master     #428       +/-   ##
===========================================
- Coverage   95.55%   57.86%   -37.70%     
===========================================
  Files           1        2        +1     
  Lines          45      159      +114     
  Branches       25       53       +28     
===========================================
+ Hits           43       92       +49     
- Misses          2       52       +50     
- Partials        0       15       +15

Impacted Files	Coverage Δ
lib/axios-extend.js	`37.50% <37.50%> (ø)`
lib/module.js	`96.36% <100.00%> (+0.80%)`	⬆️

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 a3f708d...72cb792. Read the comment docs.

An extra instance of axios is created and attached to "process.axios" so that server middlewares have access to the axios instance that has extensions provided by this module. The "proxyHeaders" option is ignored for that instance as it would be both awkward to implement and potentially wrong. This is because "proxyHeaders" is proxying requests from the request that initiated the page request while server-middleware instance is kinda independent of individual page requests. Also, it's not an entirely perfect solution because that "process.axios" instance is only extended when axios server-side plugin is executed. Ideally, it would be extended immediately when the module's code runs. That would require some refactoring to eliminate templates from the code that extends the instance and move it to a separate file. This is doable but would result in fewer byte savings. Resolves nuxt-community#427

rchl · 2020-10-15T06:31:07Z

Also, I'm thinking maybe I should use process.$axios instead to make it a bit more unique.

But let me know what you think about this whole idea in general.

pi0 · 2020-10-15T10:31:07Z

lib/module.js

@@ -121,6 +122,10 @@ function axiosModule (_moduleOptions) {
  // Set _AXIOS_BASE_URL_ for dynamic SSR baseURL
  process.env._AXIOS_BASE_URL_ = options.baseURL

+  // Create a separate instance for server middlewares.
+  // Extended from a server-side plugin.
+  process.axios = Axios.create({ baseURL: options.baseURL })


We can attach axios instance to nuxt (for other modules) and req object (for server middleware) to avoid leaking references to global

To attach to req we would only be able to do it when a request is actually made. This here creates process.axios earlier on purpose so that there is no point during runtime where server middleware would see undefined process.axios.

Hmm, I think the current solution has a problem that in the first request to server-middleware the axios instance would not be extended yet (since the server-side plugin would run after server middleware).

So that pertains to the second issue you've made below -- we would need to probably extract the "extending" code into a separate file and call it immediately rather than relying on plugin extending it.

And then putting in on req would actually be possible from the module and potentially be a better idea. It would mean that the instance of axios would be a bit harder to access (would have to be passed along various calls potentially) but workable.

pi0 · 2020-10-15T10:32:24Z

lib/plugin.js

@@ -184,6 +185,14 @@ const setupProgress = (axios) => {
  axios.defaults.onDownloadProgress = onProgress
 }<% } %>

+if (process.server) {


Why this change is necessary with this feature?

This is to extend the process.axios instance with extra methods and configuration specific to this module.

As I've hinted in the commit message, we could also extract the "extending code" into a separate file and use it both from the module and the plugin but that would require refactoring to remove the use of templates. And that means we couldn't include parts of its code conditionally, based on settings. The whole code would always be included in the bundles.

I think we can write node specific logic to create instance out of the plugin so we have clear control over behavior and also not depending on first SSR request to be done to make instance initialized

What do you mean by creating an instance out of the plugin?

Extract the code into a separate file as I've suggested or just write extra code? We basically need this part accessible from the module also:

axios-module/lib/plugin.js

Lines 6 to 113 in a3f708d

const axiosExtra = {

setBaseURL (baseURL) {

this.defaults.baseURL = baseURL

},

setHeader (name, value, scopes = 'common') {

for (const scope of Array.isArray(scopes) ? scopes : [ scopes ]) {

if (!value) {

delete this.defaults.headers[scope][name];

return

}

this.defaults.headers[scope][name] = value

}

},

setToken (token, type, scopes = 'common') {

const value = !token ? null : (type ? type + ' ' : '') + token

this.setHeader('Authorization', value, scopes)

},

onRequest(fn) {

this.interceptors.request.use(config => fn(config) || config)

},

onResponse(fn) {

this.interceptors.response.use(response => fn(response) || response)

},

onRequestError(fn) {

this.interceptors.request.use(undefined, error => fn(error) || Promise.reject(error))

},

onResponseError(fn) {

this.interceptors.response.use(undefined, error => fn(error) || Promise.reject(error))

},

onError(fn) {

this.onRequestError(fn)

this.onResponseError(fn)

},

create(options) {

return createAxiosInstance(defu(options, this.defaults))

}

}

// Request helpers ($get, $post, ...)

for (const method of ['request', 'delete', 'get', 'head', 'options', 'post', 'put', 'patch']) {

axiosExtra['$' + method] = function () { return this[method].apply(this, arguments).then(res => res && res.data) }

}

const extendAxiosInstance = axios => {

for (const key in axiosExtra) {

axios[key] = axiosExtra[key].bind(axios)

}

}

const createAxiosInstance = axiosOptions => {

// Create new axios instance

const axios = Axios.create(axiosOptions)

axios.CancelToken = Axios.CancelToken

axios.isCancel = Axios.isCancel

// Extend axios proto

extendAxiosInstance(axios)

// Setup interceptors

<% if (options.debug) { %>setupDebugInterceptor(axios) <% } %>

<% if (options.credentials) { %>setupCredentialsInterceptor(axios)<% } %>

<% if (options.progress) { %>setupProgress(axios) <% } %>

<% if (options.retry) { %>axiosRetry(axios, <%= serialize(options.retry) %>)<% } %>

return axios

}

<% if (options.debug) { %>

const log = (level, ...messages) => console[level]('[Axios]', ...messages)

const setupDebugInterceptor = axios => {

// request

axios.onRequestError(error => {

log('error', 'Request error:', error)

})

// response

axios.onResponseError(error => {

log('error', 'Response error:', error)

})

axios.onResponse(res => {

log(

'info',

'[' + (res.status + ' ' + res.statusText) + ']',

'[' + res.config.method.toUpperCase() + ']',

res.config.url)

if (process.browser) {

console.log(res)

} else {

console.log(JSON.stringify(res.data, undefined, 2))

}

return res

})

}<% } %>

<% if (options.credentials) { %>

const setupCredentialsInterceptor = axios => {

// Send credentials only to relative and API Backend requests

axios.onRequest(config => {

if (config.withCredentials === undefined) {

if (!/^https?:\/\//i.test(config.url) || config.url.indexOf(config.baseURL) === 0) {

config.withCredentials = true

}

}

})

}<% } %>

So are you suggesting to duplicate it or extract to a separate file or something else?

We may refactor useful logic into a shared CJS file as well and use both for module and runtime to avoid duplicate code like this

Yes. The only concern with that is that means the whole code will be included in the bundles rather than only code that is needed for enabled options. But I guess you are fine with that?

BTW. Is there a specific reason why you are using and suggesting comonjs syntax? I guess ES6 syntax would work with any supported Nuxt & Node version?

ES6 / Module syntax is possible for nuxt modules with esm which is deprecated. We also disable esm support for typescript and test environments. When testing another project that depends on axios module, this can be problematic.

Yes. The only concern with that is that means the whole code will be included in the bundles rather than only code that is needed for enabled options. But I guess you are fine with that?

We can mark cjs file as esm and use pure comment to allow tree-shaking per export but it shouldn't be a big deal for small shared utils.

I think ideally we should start migrating modules to typescript and proper build step to avoid such issues and also share logic in an easier way..

rchl · 2020-10-27T21:07:04Z

lib/module.js

+      const runtimeConfig = {
+        ...this.options.publicPrivateConfig,
+        ...this.options.privateRuntimeConfig
+      }


Is there a better way to access runtime config from a module (or server middleware since we are in context of one here)? Ideally with resolved values...

rchl · 2020-12-11T13:28:40Z

@pi0 What are your thoughts on this change? Any blocking issues? It's quite a big refactor so it will just keep getting broken by other changes if left unmerged.

pi0 · 2020-12-11T17:06:22Z

Hi @rchl honestly more I'm thinking it might be a tricky feature to add and maintain for axios+http. Purpose of axios module is to bind instance to renderContext not API /Config and as you mentioned most of features like proxyHeaders and progressbar integration are unused. Also usage of interceptors might be different.

Only shared parts i see are utilities and base url. Which we may use ohmyfetch for interface similar to $get util and expose $config to either req or global.nuxt.$config

So I think would introduce global instance only with http v2 (or probably @nuxt/fetch) and this way we might have a more standardized way to make requests in config and API contexts.

rchl · 2020-12-11T19:04:34Z

OK, I'll close this then and wait for something better to come in the future.

BTW. For me, the main use of this would be to define interceptors for server and client in a common config. But I can deal with those in other ways.

pi0 · 2020-12-11T19:33:13Z

@rchl Sure will keep you updated on early version.

BTW if it is about an error reporter module like sentry, would be pretty much better intercepting without directly depending on one axios/http implementation 😊

rchl · 2020-12-11T19:57:28Z

BTW if it is about an error reporter module like sentry, would be pretty much better intercepting without directly depending on one axios/http implementation 😊

The main use case for me is to have better logging of failed requests in local development.

rchl force-pushed the feat/axios-server-middleware branch from 99194a4 to cbdc9f7 Compare October 14, 2020 20:19

correct type

5cf688a

pi0 reviewed Oct 15, 2020

View reviewed changes

SebastianMieszczanczyk mentioned this pull request Oct 20, 2020

Broken server side request with cookies in 5.12.2 #411

Closed

Expose axios instance on req.$axios

79f075d

rchl commented Oct 27, 2020

View reviewed changes

Remove unneeded type import

72cb792

rchl changed the title ~~feat: create axios instance on process.axios for server-middleware use~~ feat: create axios instance on req.$axios for server-middleware use Oct 27, 2020

rchl closed this Dec 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: create axios instance on req.$axios for server-middleware use #428

feat: create axios instance on req.$axios for server-middleware use #428

rchl commented Oct 14, 2020 •

edited

Loading

codecov bot commented Oct 14, 2020 •

edited

Loading

rchl commented Oct 15, 2020

pi0 Oct 15, 2020

rchl Oct 15, 2020

rchl Oct 15, 2020

rchl Oct 15, 2020

pi0 Oct 15, 2020

rchl Oct 15, 2020

pi0 Oct 15, 2020

rchl Oct 15, 2020

pi0 Oct 15, 2020

rchl Oct 15, 2020 •

edited

Loading

pi0 Oct 15, 2020

rchl Oct 27, 2020 •

edited

Loading

rchl commented Dec 11, 2020

pi0 commented Dec 11, 2020

rchl commented Dec 11, 2020 •

edited

Loading

pi0 commented Dec 11, 2020

rchl commented Dec 11, 2020

	const axiosExtra = {
	setBaseURL (baseURL) {
	this.defaults.baseURL = baseURL
	},
	setHeader (name, value, scopes = 'common') {
	for (const scope of Array.isArray(scopes) ? scopes : [ scopes ]) {
	if (!value) {
	delete this.defaults.headers[scope][name];
	return
	}
	this.defaults.headers[scope][name] = value
	}
	},
	setToken (token, type, scopes = 'common') {
	const value = !token ? null : (type ? type + ' ' : '') + token
	this.setHeader('Authorization', value, scopes)
	},
	onRequest(fn) {
	this.interceptors.request.use(config => fn(config) \|\| config)
	},
	onResponse(fn) {
	this.interceptors.response.use(response => fn(response) \|\| response)
	},
	onRequestError(fn) {
	this.interceptors.request.use(undefined, error => fn(error) \|\| Promise.reject(error))
	},
	onResponseError(fn) {
	this.interceptors.response.use(undefined, error => fn(error) \|\| Promise.reject(error))
	},
	onError(fn) {
	this.onRequestError(fn)
	this.onResponseError(fn)
	},
	create(options) {
	return createAxiosInstance(defu(options, this.defaults))
	}
	}

	// Request helpers ($get, $post, ...)
	for (const method of ['request', 'delete', 'get', 'head', 'options', 'post', 'put', 'patch']) {
	axiosExtra['$' + method] = function () { return this[method].apply(this, arguments).then(res => res && res.data) }
	}

	const extendAxiosInstance = axios => {
	for (const key in axiosExtra) {
	axios[key] = axiosExtra[key].bind(axios)
	}
	}

	const createAxiosInstance = axiosOptions => {
	// Create new axios instance
	const axios = Axios.create(axiosOptions)
	axios.CancelToken = Axios.CancelToken
	axios.isCancel = Axios.isCancel

	// Extend axios proto
	extendAxiosInstance(axios)

	// Setup interceptors
	<% if (options.debug) { %>setupDebugInterceptor(axios) <% } %>
	<% if (options.credentials) { %>setupCredentialsInterceptor(axios)<% } %>
	<% if (options.progress) { %>setupProgress(axios) <% } %>
	<% if (options.retry) { %>axiosRetry(axios, <%= serialize(options.retry) %>)<% } %>

	return axios
	}

	<% if (options.debug) { %>
	const log = (level, ...messages) => console[level]('[Axios]', ...messages)

	const setupDebugInterceptor = axios => {
	// request
	axios.onRequestError(error => {
	log('error', 'Request error:', error)
	})

	// response
	axios.onResponseError(error => {
	log('error', 'Response error:', error)
	})
	axios.onResponse(res => {
	log(
	'info',
	'[' + (res.status + ' ' + res.statusText) + ']',
	'[' + res.config.method.toUpperCase() + ']',
	res.config.url)

	if (process.browser) {
	console.log(res)
	} else {
	console.log(JSON.stringify(res.data, undefined, 2))
	}

	return res
	})
	}<% } %>

	<% if (options.credentials) { %>
	const setupCredentialsInterceptor = axios => {
	// Send credentials only to relative and API Backend requests
	axios.onRequest(config => {
	if (config.withCredentials === undefined) {
	if (!/^https?:\/\//i.test(config.url) \|\| config.url.indexOf(config.baseURL) === 0) {
	config.withCredentials = true
	}
	}
	})
	}<% } %>

feat: create axios instance on req.$axios for server-middleware use #428

feat: create axios instance on req.$axios for server-middleware use #428

Conversation

rchl commented Oct 14, 2020 • edited Loading

codecov bot commented Oct 14, 2020 • edited Loading

Codecov Report

rchl commented Oct 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchl Oct 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchl Oct 27, 2020 • edited Loading

Choose a reason for hiding this comment

rchl commented Dec 11, 2020

pi0 commented Dec 11, 2020

rchl commented Dec 11, 2020 • edited Loading

pi0 commented Dec 11, 2020

rchl commented Dec 11, 2020

rchl commented Oct 14, 2020 •

edited

Loading

codecov bot commented Oct 14, 2020 •

edited

Loading

rchl Oct 15, 2020 •

edited

Loading

rchl Oct 27, 2020 •

edited

Loading

rchl commented Dec 11, 2020 •

edited

Loading