Advanced middleware feature #23

iamlacroix · 2017-02-23T23:42:23Z

This is the advanced middleware implementation per our discussion in #4.

It moves the built-in superagent request object into an adapter and that adapter is used by default importing the standard queryMiddleware. A new queryMiddlewareAdvanced export is available which requires an adapter as one of the parameters.

The adapter and it's initializer look like this:

type Adapter = {
  execute: Function,
  abort: Function,
}

type CreateAdapter = (
  url: string,
  method: 'GET' | 'POST' | 'PUT' | 'DELETE',
  config?: { body?: string | Object, headers?: Object } = {},
) => Adapter

Once/if all this is close to being merged, I can update the documentation in the README as well (for advanced usage, normal usage remains the same).

codecov-io · 2017-02-23T23:44:52Z

Codecov Report

Merging #23 into master will increase coverage by 2.27%.
The diff coverage is 94.81%.

@@            Coverage Diff             @@
##           master      #23      +/-   ##
==========================================
+ Coverage   90.14%   92.41%   +2.27%     
==========================================
  Files           7        9       +2     
  Lines         213      211       -2     
==========================================
+ Hits          192      195       +3     
+ Misses         21       16       -5

Impacted Files	Coverage Δ
src/middleware/query.js	`100% <100%> (+8.75%)`	✅
src/middleware/query-advanced.js	`94.54% <94.54%> (ø)`
src/adapters/superagent.js	`95.45% <95.45%> (ø)`

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 ba5e669...ae29cf4. Read the comment docs.

ryanashcraft

@iamlacroix Thanks for taking the time to pull this together. I have a few comments. Let me know what you think.

ryanashcraft · 2017-02-27T02:42:32Z

src/adapters/superagent.js

+import superagent from 'superagent';
+import * as httpMethods from '../constants/http-methods';
+
+export const __createRequest = (url, method) => {


Is this to indicate that it shouldn't be used except by tests? If so I think it's fine to just export it without the __ prefix.

Yea that's all it meant, I'll remove it.

ryanashcraft · 2017-02-27T02:45:15Z

src/middleware/query-advanced.js

+
+const resOk = (status) => Math.floor(status / 100) === 2;
+
+const queryMiddlewareAdvanced = (queriesSelector, entitiesSelector, networkAdapter, config = defaultConfig) => {


If we pull networkAdapter out like so:

const queryMiddlewareAdvanced = (networkAdapter) => (queriesSelector, entitiesSelector, config = defaultConfig)

Then the default middleware will be much simpler:

const queryMiddleware = queryAdvanced(superagentAdapter);

🤦‍♂️ That's a much better idea.

ryanashcraft · 2017-02-27T02:54:02Z

src/index.js

@@ -8,5 +8,6 @@ export { default as getQueryKey } from './lib/get-query-key';
 export { default as queriesReducer } from './reducers/queries';
 export { default as entitiesReducer } from './reducers/entities';
 export { default as queryMiddleware } from './middleware/query';
+export { default as queryMiddlewareAdvanced } from './middleware/query-advanced';


The problem I see with this is that projects that use the new queryMiddlewareAdvanced will keep pulling in superagent through the queryMiddleware from above.

I'm thinking we can easily fix this by:

duplicating src/index.js to a new file src/advanced.js

removing the './middleware/query' import in src/advanced.js

removing the './middleware/query-advanced' import in src/index.js (to distinguish the different entries and usages)

add a file to the root of the project (advanced.js) that is a CommonJS alias to dist/commonjs/advanced

This is sort of brittle because a single import for regular redux-query will cause the app's bundler to start including superagent, but I think it's acceptable for now. What do you think?

Additionally I think we can modify the webpack config to export a new UMD build for redux-query/advanced.

iamlacroix · 2017-02-27T17:09:45Z

@ryanashcraft Thanks for your feedback! I've updated the PR accordingly. Are the UMD changes I made what you had in mind? If not, I'll be happy to update it.

This moves most of the logic from the query middleware to the new advanced middleware. The query middleware will call advanced using the default superagent adapter.

ryanashcraft · 2017-02-27T19:02:03Z

@iamlacroix Sorry I botched a minor change (ae29cf4) I added to this PR and had to do a force push.

The UMD builds look good. I'm ready to merge this. Before I do, can you do some testing with your app to make sure the advanced entry point and custom adapters works as expected? Also, if you can update add some info in the README that'd be awesome. Thanks!

iamlacroix · 2017-02-28T16:52:14Z

@ryanashcraft Great to hear! I'll update the README and let you know the results of testing as soon as I can.

jnutter · 2017-03-07T21:10:17Z

Can we add support for PATCH as a verb?

ryanashcraft · 2017-03-08T16:51:11Z

@jnutter Yes, that should be easy but somewhat orthogonal to this PR. I made a new issue to track that here: #33.

ryanashcraft · 2017-03-11T18:13:33Z

@iamlacroix Do you mind if I help out with this PR to get it merged? I actually need the advanced middleware for a project I have in mind. 😊

iamlacroix · 2017-03-13T01:27:33Z

@ryanashcraft Yea, absolutely. Sorry for the delay on my end, I was sick for a bit and have been getting caught up on work.

…edux-query into iamlacroix-advanced-middleware

…roix-advanced-middleware

acontreras89 · 2017-03-30T23:55:33Z

Sorry to write here but I didn't know where to ask.

I've recently developed an adapter for fetch. Fetch doesn't support request abortion yet, so it will require more work eventually.

I was wondering if this is something you'd like to add to this project (as an alternative to the SuperAngent adapter), or it would better fit its own separate package.

ryanashcraft · 2017-03-31T15:51:45Z

@acontreras89 Nice! I think for now it makes more sense to have it as a separate package.

iamlacroix mentioned this pull request Feb 23, 2017

Proof-of-concept for custom network adapters #4

Closed

ryanashcraft reviewed Feb 27, 2017

View reviewed changes

iamlacroix and others added 4 commits February 27, 2017 10:58

Add advanced middleware feature to inject custom network adapters

80122aa

This moves most of the logic from the query middleware to the new advanced middleware. The query middleware will call advanced using the default superagent adapter.

Add tests for the superagent adapter

18dd776

Create separate default and advanced entries

ee87e7f

Add advanced CommonJS entry point alias

ae29cf4

ryanashcraft force-pushed the advanced-middleware branch from 09b08c9 to ae29cf4 Compare February 27, 2017 18:58

ryanashcraft mentioned this pull request Feb 28, 2017

Expose normalised data into async response #25

Merged

ryanashcraft added 5 commits March 18, 2017 16:08

Merge branch 'advanced-middleware' of https://github.com/iamlacroix/r…

fe2d91d

…edux-query into iamlacroix-advanced-middleware

Merge branch 'master' of github.com:amplitude/redux-query into iamlac…

07b1239

…roix-advanced-middleware

Fix advanced invalid imports

629dcc5

Add test for PATCH in superagent adapter

2c901f3

Update README with docs about redux-query/advanced

07246ae

ryanashcraft merged commit d922161 into amplitude:master Mar 19, 2017

ryanashcraft mentioned this pull request Mar 19, 2017

Add headers to return #42

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Advanced middleware feature #23

Advanced middleware feature #23

iamlacroix commented Feb 23, 2017

codecov-io commented Feb 23, 2017 •

edited

ryanashcraft left a comment

ryanashcraft Feb 27, 2017

iamlacroix Feb 27, 2017

ryanashcraft Feb 27, 2017

iamlacroix Feb 27, 2017

ryanashcraft Feb 27, 2017

ryanashcraft Feb 27, 2017

iamlacroix commented Feb 27, 2017

ryanashcraft commented Feb 27, 2017

iamlacroix commented Feb 28, 2017

jnutter commented Mar 7, 2017

ryanashcraft commented Mar 8, 2017

ryanashcraft commented Mar 11, 2017

iamlacroix commented Mar 13, 2017

acontreras89 commented Mar 30, 2017 •

edited

ryanashcraft commented Mar 31, 2017


		const resOk = (status) => Math.floor(status / 100) === 2;

		const queryMiddlewareAdvanced = (queriesSelector, entitiesSelector, networkAdapter, config = defaultConfig) => {

Advanced middleware feature #23

Advanced middleware feature #23

Conversation

iamlacroix commented Feb 23, 2017

codecov-io commented Feb 23, 2017 • edited

Codecov Report

ryanashcraft left a comment

Choose a reason for hiding this comment

ryanashcraft Feb 27, 2017

Choose a reason for hiding this comment

iamlacroix Feb 27, 2017

Choose a reason for hiding this comment

ryanashcraft Feb 27, 2017

Choose a reason for hiding this comment

iamlacroix Feb 27, 2017

Choose a reason for hiding this comment

ryanashcraft Feb 27, 2017

Choose a reason for hiding this comment

ryanashcraft Feb 27, 2017

Choose a reason for hiding this comment

iamlacroix commented Feb 27, 2017

ryanashcraft commented Feb 27, 2017

iamlacroix commented Feb 28, 2017

jnutter commented Mar 7, 2017

ryanashcraft commented Mar 8, 2017

ryanashcraft commented Mar 11, 2017

iamlacroix commented Mar 13, 2017

acontreras89 commented Mar 30, 2017 • edited

ryanashcraft commented Mar 31, 2017

codecov-io commented Feb 23, 2017 •

edited

acontreras89 commented Mar 30, 2017 •

edited