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

not working when browser not support Symbol #22

Closed
ariesjia opened this issue Nov 12, 2015 · 28 comments
Closed

not working when browser not support Symbol #22

ariesjia opened this issue Nov 12, 2015 · 28 comments

Comments

@ariesjia
Copy link

if browser not support Symbol ,the action.key is string not equal to object , so push error and not send api

for (let key in action) {
    if (key !== [CALL_API]) {
      validationErrors.push(`Invalid root key: ${key}`);
    }
  }

I fount if browser support Symbol , do not enter this for loop . because action.key is real Symbol

Maybe this validate is not necessary .

@ariesjia ariesjia changed the title not not working when browser not support Symbol Nov 12, 2015
@olslash
Copy link

olslash commented Dec 4, 2015

+1 -- having this issue in phantom, even though it's going through babel first. Any ideas?

@olslash
Copy link

olslash commented Dec 4, 2015

as a hotfix in my codebase, i fixed this by:

there is some weird issue with the way babel is handling symbols that's preventing it from working, but I can't figure it out.

@riophae
Copy link

riophae commented Dec 11, 2015

You could inject a polyfill for Symbol into your project.
See: https://github.com/zloirock/core-js#commonjs

@olslash
Copy link

olslash commented Dec 12, 2015

I changed my test setup to not include Phantom so I can't verify this, but I believe I did have Symbol polyfilled. It's possible the issue was with the polyfill, or that it wasn't working properly, although other ES6 stuff like Promise did. Once i get browser tests running again I'll report back.

@riophae
Copy link

riophae commented Dec 12, 2015

@olslash Did you bundle the project when testing in Phantom.js? As far as I know, if you didn't use something like Browserify, Phantom.js could work improperly with core.js polyfills.

@jamesbillinger
Copy link

+1 I'm having this issue in IE11. I added es6-symbol and verified that I can create and use Symbols, but I still can't get [CALL_API] to work in IE.

@stinju
Copy link

stinju commented Dec 19, 2015

FWIW, I ran into the same issue when using with react-native on pre-iOS-9 devices.

@danny-andrews
Copy link

I don't think we should be using symbols in the action anyway, as the redux docs suggest against it: "We recommend that you use strings and not Symbols for action types, because strings are serializable, and by using Symbols you make recording and replaying harder than it needs to be." Although this is talking specifically about action types, the same principle applies for all parts of an action.

So I think we should change the lines here from:

const CALL_API = Symbol('Call API');

export default CALL_API;

to

export default 'CALL API';

@geminiyellow
Copy link

@agraboso

i found the Discussion about use Symbol as action type.

reduxjs/redux#114 (comment)
reduxjs/redux#127

and @gaearon change Symbol to prefix @@ as initial action special type in redux.
so how about change Symbol('Call API') to redux-api-middleware/API or @@redux-api-middleware/API or something else.

@ariesjia
Copy link
Author

+1 for '@@'

@barrystaes
Copy link
Contributor

Agreed.

Its also what react-router-redux does: @@router/UPDATE_LOCATION
(formerly known as redux-simple-router)

@barrystaes
Copy link
Contributor

@geminiyellow
I guess now the hard part is naming things, and do a PR.. and update docs and tests.

I think CALL_API is fine, but what about using @@redux-api-middleware/RSAA as the value, and RSAA as the value's name? This would make it somewhat self-explaining and future proof.

The example would then become

import { RSAA } from `redux-api-middleware`;

{
  [RSAA]: {
    endpoint: 'http://www.example.com/api/users',
    method: 'GET',
    types: ['REQUEST', 'SUCCESS', 'FAILURE']
  }
}

Just a thought.. i have to say that CALL_API is also quite self-explanatory..

@geminiyellow
Copy link

@barrystaes
yes, i think so, CALL_API is good enough, but RSAA is better.
[CALL_API] in redux-api-middleware is a type name,
so nouns RSAA looks more self-explaining, and shorter, and no _.
i dont like _ :trollface:

sorry, @georgebonnr , wrong @

@georgebonnr
Copy link
Contributor

  1. 👍 this sounds good to me.
  2. Did you mean to @ mention me in this thread? Did my username get autofilled in because I've forked?
  3. This project is rad, btw, and I'm loving it in production at Patreon. I've been able to build some utils on top of this that we might share back soon. Really simplified our data fetching story! 🍻

@geminiyellow
Copy link

oh My fault, sorry @georgebonnr .
i just want to reply @barrystaes 's #22 (comment)

@stepan-romankov
Copy link

+1 It's nice to have it working on IE8

@barrystaes
Copy link
Contributor

@georgebonnr I'm looking forward to your experiences and utils. :)
If its opensource, i would be happy to link these under a "Projects that use redux-api-middleware" section in the README.md file.

edit: i just added a projects section to the README.md

@jackyon
Copy link

jackyon commented Mar 2, 2016

so what is the best solution for fixing this issue?

@agraboso agraboso mentioned this issue Mar 2, 2016
2 tasks
@barrystaes
Copy link
Contributor

PR #43 is the best solution, and soon wil be in the next branch for v2.0.0-beta.

@jackyon
Copy link

jackyon commented Mar 8, 2016

@barrystaes

thx for reminding, can't wait for the next release version.

barrystaes added a commit that referenced this issue Mar 8, 2016
[MODIFIED] Fix for issue #22, updated Babel and ESLint to latest (test script was failing after cloning fresh repo)
@gilbarbara
Copy link

@barrystaes Hey!
Can you publish the next branch to npm @next?

[s]

@jackyon
Copy link

jackyon commented Mar 17, 2016

+1

@barrystaes
Copy link
Contributor

I can not publish to npm, but @agraboso can.

A workaround is to use [git-urls-as-dependencies](https://docs.npmjs.com/files/package.json#git urls as dependencies).
"redux-api-middleware": "git://github.com/agraboso/redux-api-middleware.git#next"

@gilbarbara
Copy link

@barrystaes
I know this but there's no /lib in the git repo, so it will not work (I've tried..). I'm using a local compiled version but it's lame.

@agraboso Can you please publish to npm or something? Thanks

@gaearon
Copy link

gaearon commented Apr 24, 2016

Lol, I feel bad for putting a symbol there in the first place in the real-world example. 😄

@gilbarbara
Copy link

ok, let's blame @gaearon 😁

@agraboso
Copy link
Owner

I just published v1.0.2 and v2.0.0-beta.1 to npm.

@karladler
Copy link

karladler commented Apr 3, 2017

I still get the error in IE11 using version 1.0.2 is this already released there? What would be the best solution to get that fixed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests