agraboso / redux-api-middleware

Redux middleware for calling an API.
MIT License
1.49k stars 195 forks source link

Fix default fetch function #241

Closed essensx closed 4 years ago

essensx commented 4 years ago

https://github.com/agraboso/redux-api-middleware/issues/219

This small change adds support for using this package in node environment

coveralls commented 4 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling eb26df3c88cf897da9742a4fd3cc7b47ac937315 on essensx:bugfix/default-fetch into 4b87ccca84710991f0876d0c051e5e0ff223ac99 on agraboso:master.

iamandrewluca commented 4 years ago

This may throw a error in browser

global.fetch
VM289:1 Uncaught ReferenceError: global is not defined
    at <anonymous>:1:1

I would propose to use createMiddleware with your own fetch compilant function https://github.com/agraboso/redux-api-middleware#createmiddlewareoptions

essensx commented 4 years ago

global should be resolved on build, depending on build output. Build tools should resolve global to window in the browser.

createMiddleware will not do the job, as the build fails because of undefined fetch on node env.

nason commented 4 years ago

Interesting. I've used this middleware in node, but always with https://www.npmjs.com/package/isomorphic-fetch so it "just worked". I think this is a good change, and makes sense to me.

You're right the build step should take care of resolving global for browsers. How about the UMD build? I think it should be ok too because of the way it will be wrapped ie, ((global) => { middlware! })(window)

This may throw a error in browser

It shouldn't as long as our build tools don't fail us :) @iamandrewluca assuming that's the case, do you see any other issues here?

essensx commented 4 years ago

@nason ping

iamandrewluca commented 4 years ago

@essensx I'll take a look in weekend.

karolis-sh commented 4 years ago

Any updates? 😅

iamandrewluca commented 4 years ago

This will break UMD build. Right now at build time global is not resolved. We have to update babel dev dependencies and use globalThis https://github.com/tc39/proposal-global I assigned this to me.

essensx commented 4 years ago

Hmm, nice catch, i thought this should be resolved in umd builds, as output actually contains resolver for global, which happens to be for internal usage. If you don't find the time, i can update the PR with updated babel devDeps and proposal implementation

iamandrewluca commented 4 years ago

@essensx I'm working already on this, have to upgrade jest, babel, and rollup to lateste verions. I'll ping you when is ready.

iamandrewluca commented 4 years ago

@essensx I updated most of dev dependencies, except jest. You can try to play with babel config, to include global-this. I tried it works but CJS bundle size increases a lot.

docs: https://babeljs.io/docs/en/babel-preset-env

You can see what I tried here https://github.com/iamandrewluca/redux-api-middleware/commit/5f5e06330b330a4e05950b048e7bc78cfe749705

aluca@acula:~/Projects/github.com/iamandrewluca/redux-api-middleware|elena 
⇒  npm run build

> redux-api-middleware@3.2.0 build /home/aluca/Projects/github.com/iamandrewluca/redux-api-middleware
> babel src --out-dir es && rollup -c

Successfully compiled 10 files with Babel.

src/index.js → lib/index.umd.js...
created lib/index.umd.js in 1.3s

src/index.js → lib/index.cjs.js...
created lib/index.cjs.js in 678ms

> redux-api-middleware@3.2.0 postbuild /home/aluca/Projects/github.com/iamandrewluca/redux-api-middleware
> npm run size

> redux-api-middleware@3.2.0 size /home/aluca/Projects/github.com/iamandrewluca/redux-api-middleware
> size-limit

  lib/index.cjs.js (min)
  Package size limit has exceeded by 26.14 KB
  Size limit: 10 KB
  Size:       36.14 KB with all dependencies and minified

  lib/index.cjs.js (min + gzip)
  Package size limit has exceeded by 8.48 KB
  Size limit: 4 KB
  Size:       12.48 KB with all dependencies, minified and gzipped

  lib/index.umd.js (min)
  Package size limit has exceeded by 5.17 KB
  Size limit: 31 KB
  Size:       36.17 KB with all dependencies and minified

  lib/index.umd.js (min + gzip)
  Package size limit has exceeded by 532 B
  Size limit: 12 KB
  Size:       12.52 KB with all dependencies, minified and gzipped

  Try to reduce size or increase limit at .size-limit
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! redux-api-middleware@3.2.0 size: `size-limit`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the redux-api-middleware@3.2.0 size script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/aluca/.npm/_logs/2020-02-24T16_24_54_505Z-debug.log
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! redux-api-middleware@3.2.0 postbuild: `npm run size`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the redux-api-middleware@3.2.0 postbuild script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/aluca/.npm/_logs/2020-02-24T16_24_54_512Z-debug.log
aluca@acula:~/Projects/github.com/iamandrewluca/redux-api-middleware|elena
essensx commented 4 years ago

@iamandrewluca the huge bundle size increase is due to our old node target (v8), since there are more proposals that needs polyfilling due to updated babel env. I suggest we bump node target version to at least 10.15.0 (result https://i.imgur.com/gLTjwLB.png).

Otherwise, we need to selectively include babel proposals, instead of using preset-env

essensx commented 4 years ago

@iamandrewluca what are your thoughts? Looks like node 8.x reached Its end of life.

iamandrewluca commented 4 years ago

@nason your thoughts? I am ok with dropping node@8

essensx commented 4 years ago

@nason ping

nason commented 4 years ago

@iamandrewluca @essensx lets drop node 8!

essensx commented 4 years ago

@iamandrewluca Will you update the PR you shared (a few lines), or should i fork it and update this one?

iamandrewluca commented 4 years ago

@essensx you can give a try!

Neodern commented 4 years ago

Any update for this PR ? i'm also stuck with redux-api-middleware used with a node context and a "fetch is not defined exception" :(

iamandrewluca commented 4 years ago

Released in https://github.com/agraboso/redux-api-middleware/releases/tag/v3.2.1

iamandrewluca commented 4 years ago

Droping node@8 I think should imply a major version release for redux-api-middleware