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

Fix for issue #22, updated Babel and ESLint to latest (test script was failing after cloning fresh repo) #43

Closed
wants to merge 8 commits into from

Conversation

jtribble
Copy link
Contributor

After cloning the repo, I ran into a failure when running the test script. It seemed to be an issue with the environment somehow, not with a test failing, so I performed the following changes:

  • Updated Babel and ESLint to latest.
  • Reconfigured Babel to use Babel 6 workflow.
  • Modified an ESLint setting (my lint was failing because ESLint was reading rules from a parent repo).

After I had all the tests passing and verified that stuff was working, I performed the following changes:

  • Made the global switch from CALL_API (Symbol) to RSAA (String).
  • Updated docs to reflect the changes.

Let me know if this works! Not sure how this would be versioned, since it's a breaking change.

@barrystaes
Copy link
Contributor

Nice work Jeff !
I shall test it, expect me to come back on this next week.

If this breaks projects using it, we should either

  • make this a 2.x branch, and take the opportunity to combine it with other TBD breaking changes. Meanwhile, we might want to release a final 1.0.0 before releasing a 2.x version.. @agraboso ?
    This 2.x branch should get a "upgrading from 1.x" section in the README.md
  • leave support in for import { API_CALL } from 'redux-api-middleware' but warn that its deprecated.

@barrystaes
Copy link
Contributor

Mentioning #22 so that it gets linked.

@jtribble
Copy link
Contributor Author

Cool, sounds like a plan! Let me know if there's anything I can do to help.

@agraboso
Copy link
Owner

@barrystaes I concur there should be a v1.0.0 before getting these breaking changes in. Can you email me at agraboso@gmail.com so we can arrange for me to give you access to the npm package?

@jtribble I see you've published your fork to npm as redux-api-middleware-rsaa-string. Hopefully you can work with @barrystaes to reconcile it with redux-api-middleware.

@jtribble
Copy link
Contributor Author

@agraboso - The only reason I published the fork on npm was so that I could use it in a production app, but in retrospect I could have just loaded the module from a different location, outside of npm. I'd be happy to modify my project and remove the duplicate npm package so that there aren't two versions floating around.

@agraboso
Copy link
Owner

@jtribble No worries: I completely understand. I'm glad you're using redux-api-middleware. I'm sorry I don't have the time to maintain it, but @barrystaes is picking it up very diligently and I'm sure he's happy to have you contribute.

@jtribble
Copy link
Contributor Author

@agraboso - It's a sweet tool, I love it! Contributing is fun too.

Just unpublished my forked npm package, hopefully didn't screw anyone over.. oO

@barrystaes
Copy link
Contributor

I guess in while the final 1.0.0 release gets formulated, we could make a npm prerelease for testing by @jtribble and me.

I was pondering to make a branch 2.x, accept PR #22 in it, release that branch to npm as npm publish . --tag 2.0.0-beta1.
This would let anyone install it as npm install redux-api-middleware@2.0.0-beta1.
Ok?

@jackyon
Copy link

jackyon commented Mar 2, 2016

can't wait for the new release. Now my project can not run on ie browsers, this give me a lot of headache.

@agraboso
Copy link
Owner

agraboso commented Mar 2, 2016

v1.0.0 is now published on npm.

@barrystaes: there was a test that failed in my machine because I had Apache running and the test isn't mocking the appropriate endpoint — it succeeded once I turned Apache off. It is the test entitled apiMiddleware must dispatch an error request FSA on a request error starting in line 1092 of text/index.js. The weird thing is that I tried adding

  const api = nock('http://127.0.0.1')
                .get('/api/users/1')
                .reply(404);

at the beginning of the test and it kept failing, but succeeded if I switched 404 for 301. Something to have a look at for the future.

@agraboso agraboso closed this Mar 2, 2016
@agraboso
Copy link
Owner

agraboso commented Mar 2, 2016

My apologies for closing this by mistake. I meant to post in #45.

@agraboso agraboso reopened this Mar 2, 2016
@agraboso agraboso mentioned this pull request Mar 2, 2016
2 tasks
@jcs12311
Copy link

jcs12311 commented Mar 2, 2016

@jtribble after i change to your fork version, the ie brower 11 not working, the CALL_API issue was solve, but got another problem, plz see the below attachment:
zzzavvzz0160302111622

@agraboso @barrystaes
after i upgrade to the v1.0.0.0, the ie browser 11 still not working, because the CALL_API Symbol issue i think.

plz advice how to solve it.

@jtribble
Copy link
Contributor Author

jtribble commented Mar 2, 2016

@jcs12311 - Could you post a screenshot of the error message you're seeing?

@jcs12311
Copy link

jcs12311 commented Mar 2, 2016

@jtribble im sorry... i have not got any error on IE developer tool, just alert trigger fail.

@jtribble
Copy link
Contributor Author

jtribble commented Mar 2, 2016

@jcs12311 - Hmm.. So when you run your code in IE11, you get a "fail" alert message, but you get "req" or "suc" when running in other browsers?

You could try changing your failure RSAA to look something like this, and see what the values of the params are:

{
  type: CONSTANTS.NEWS_FAILURE,
  payload: (action, state, res) => {
    console.log('action', action);
    console.log('state', state);
    console.log('res', res);
  }
}

@jcs12311
Copy link

jcs12311 commented Mar 2, 2016

@jtribble Sorry for my explaination, the issue on IE 11 is CONSTANTS.NEWS_REQUEST's payload was be called, CONSTANTS.NEWS_SUCCESS and CONSTANTS.NEWS_FAILURE payload were never called.
In Chrome, every thing is normally worked.

@jcs12311
Copy link

jcs12311 commented Mar 8, 2016

I found the problem is IE didn't support Promise, i install es6-promise, the problem solved.

@barrystaes
Copy link
Contributor

@jcs12311 Thanks for the clarification.

@jtribble can you retarget the PR to the next branch for v2.0.0-beta? If not i'll happily merge it myself, but GitHub wont understand what happened to this PR. Not sure if thats a problem tho..

@barrystaes barrystaes mentioned this pull request Mar 8, 2016
@jtribble
Copy link
Contributor Author

jtribble commented Mar 8, 2016

@barrystaes - Sure thing, I opened a new PR and targeted it at the next branch: #52

I think we can close this PR once #52 is good to go.

@barrystaes
Copy link
Contributor

Ok thanks.

@barrystaes barrystaes closed this Mar 8, 2016
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.

6 participants