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

Redux 4 breaking changes #1342

Closed
8 tasks done
gaearon opened this issue Feb 1, 2016 · 33 comments
Closed
8 tasks done

Redux 4 breaking changes #1342

gaearon opened this issue Feb 1, 2016 · 33 comments
Milestone

Comments

@gaearon
Copy link
Contributor

gaearon commented Feb 1, 2016

Creating this issue to keep a pile of breaking changes we want to release at once. This doesn’t mean we plan to do much breaking changes! Mostly they are cosmetic but since we’re using semver, we want to batch them. This is why this issue exists.

@timdorr
Copy link
Member

timdorr commented Feb 1, 2016

we now support importing redux/lib/*

We do? I always viewed any import from outside of main (basically any import with a slash) as accessing private APIs. Your main should be the list of public APIs provided by the library. The only exception is for reducing your bundle size (until ES6 module builders become more readily available), but you would be doing this as an optimization step after importing something from the public API.

Drop support for IE8.

Then we can import #1123 finally? 😄

@gaearon
Copy link
Contributor Author

gaearon commented Feb 1, 2016

The only exception is for reducing your bundle size (until ES6 module builders become more readily available), but you would be doing this as an optimization step after importing something from the public API.

Yes, since 3.0.6. /lib/ is a practical way to do it today and I see no reason to disallow it until ES6-aware bundlers are commonplace. Our top-level API is stable so I don’t expect supporting it to be a pain.

Then we can import #1123 finally?

I’m looking into getting this in with es3ifyPlugin for now. But future-wise, yeah.

@timdorr
Copy link
Member

timdorr commented Feb 1, 2016

Our top-level API is stable so I don’t expect supporting it to be a pain.

Yeah, I don't think it's a pain thing either, just more of a philosophical issue. How do we want to go about importing them for combineReducers? Move them to utils and import from there?

@timdorr
Copy link
Member

timdorr commented Feb 1, 2016

Welp, I just went ahead and did that as a PR 😄 If you wanted to do it another way, no worries.

@gaearon
Copy link
Contributor Author

gaearon commented Feb 1, 2016

Move them to utils and import from there?

👍

@guijesuis
Copy link

I d like to share a little bit of my experience with redux. i spent 5 month working on a fairly large redux application.
Redux did an awesome job. Nevertheless after few weeks of dev we hit the issue "how to deal with asynchronous actions". At first we used redux thunk and it did the job, but ours actions grew rapidly in complexity.
At that point i decided to create a middleware base and generator functions (similar to redux saga but it was still actions). it allowed us to write complex action like the following login action

export function* login(login, password) {
  var {state} = yield {
    type: types.ON_DO_NOTHING
  };

  if (state.cursor == "wait")
    return {type: types.ON_DO_NOTHING};

  yield {type: types.ON_WAITCURSOR_START};

  var {state,result} = yield RestManager.Call({
    method: REST.METHODS.POST,
    url: REST.RESOURCES.SESSIONS,
    header: RestManager.getAuthenticationHeader(login, password)
  });

  if (result.status == "success") {

    yield  {
      type: types.ON_REQUEST_SESSION_ID_COMPLETED,
      sessionID: result.data.content.sessionId,
    };

    var {state,result} = yield RestManager.Call({
      method: REST.METHODS.GET,
      url: REST.RESOURCES.FEATURES + "/AIPLM_INTEGRATION/users/" + login,
      header: RestManager.getBasicHeader(REST.APPLICATION_TYPE.JSON)
    });

    if (result.status == "success") {

      yield {type: types.ON_GET_CURRENT_DOCUMENT};
    }
    else {

      yield {type: types.ON_DISCONNECT_YES};

      yield messagesAction.displayMessage("error", i18n.t("NO_FEATURE_MESSAGE"));
    }
  }
  else {

    yield messagesAction.displayMessage("error", result.data.content);
  }

  return {type: types.ON_WAITCURSOR_STOP};
}

And that how we resolved our issue with asynchronous action..

I now believe that redux is mixing 2 distinct concept as one, action and store effect.
• Actions should be what the user or computer trigger (ex: click on button, clock-tic , post form ...)
• Store effects should be triggered by action to mutate the store

@gaearon
Copy link
Contributor Author

gaearon commented Feb 12, 2016

@guijesuis

Awesome that you discovered this pattern while working on a real world app. I agree with you that this seems like a great idea. In fact this is exactly what https://github.com/yelouafi/redux-saga offers.

@gaearon
Copy link
Contributor Author

gaearon commented Feb 12, 2016

Oh wait, you mentioned that. Yeah it’s not exactly the same now that I look at it. This is both the blessing and the curse of Redux: you can choose any abstractions that you feel comfortable with.

@guijesuis
Copy link

Redux-saga grant daemon. within daemon you can track action and dispatch effect (but those effect are still action in redux). Don't you believe that separating those 2 (getting a store effect dispatcher and an action effect dispatcher with their own midleware pipe) one way to improve redux in the future.

@taion
Copy link
Contributor

taion commented Sep 4, 2016

BTW, I think imports of the form ${package}/lib/${module} are not ideal, though they're the easiest way to go. Especially if you have a separate ES module build for which tree shaking works anyway, you should probably expose those as ${package}/${module} imports, potentially by npm publishing a subdirectory rather than the project's Git root.

@markerikson
Copy link
Contributor

@monolithed : not sure what you're actually asking for. Thunks are pretty simple: just functions returning functions. And those aren't even a required part of Redux.

@monolithed
Copy link

monolithed commented Sep 23, 2016

@markerikson

not sure what you're actually asking for.

Just thinking out loud 😄

just functions returning functions

Yep, I meant store => next => action => ... and other carried functions. I feel pain in my eyes when I see that... 😪

And those aren't even a required part of Redux.

I see

@timdorr
Copy link
Member

timdorr commented Sep 23, 2016

Those aren't carried functions. They're called higher order functions. Similar to higher order components. They're a means of encapsulation of both data and logic. And arrow functions make them quite palatable :)

@rsolomon
Copy link

I noticed that TypeScript is on that checklist but not Flow. Any chance someone in the know could work that in there? 🙏

@timdorr
Copy link
Member

timdorr commented Nov 18, 2016

@rsolomon Already done in #1887

@aikoven
Copy link
Collaborator

aikoven commented Nov 20, 2016

We can also migrate typings to TypeScript 2.1, introduce null-safety and strict mapped types for combineReducers.

@muudyguy
Copy link

muudyguy commented Jan 1, 2017

Oh man I wish you have said, "we don't support IE" instead of mentioning the version.

@Jessidhia
Copy link

Why?

The most extreme option that would still be reasonable would be dropping support for IE < 11. All modern JS features (but Proxy and new.target) can be polyfilled in it.

IE8, OTOH, is a risk because of how easy it is to crash when using Object static methods... which happens all the time in transpiled classes and modules.

@muudyguy
Copy link

muudyguy commented Jan 2, 2017

Because the only reason they are still used, is that everyone keeps supporting them. Users of internet explorer is not gonna stop using computers if suddenly internet explorer stops working. It is a waste of time to give a moment of thought to how things would work in IE. I don't understand why it was worth a thumbsdown.

@fhelwanger
Copy link

But sometimes you must made something that needs to run on IE. If redux don't support that, redux won't be used in these projects. If that was the case, I couldn't use redux at work. And IE11 isn't THAT bad at all.

@jimbolla
Copy link
Contributor

jimbolla commented Jan 2, 2017

IE still has a large marketshare, especially in US corporate environment. No major general-purpose JS package is going to drop support for it completely. This isn't being considered for Redux and is off-topic.

@monolithed
Copy link

monolithed commented Jan 2, 2017

Why do you mention IE? IE8 released 8 years ago and I think we should not care about that. If you really want IE8 stay on the previous version.

@timdorr
Copy link
Member

timdorr commented Jan 2, 2017

Folks, we only said we're dropping support for IE8. No other version. If you need support for that version, you can continue to use 3.x. We won't be unpublishing that version on release of 4.0 and it will continue to work just fine, as it does right now.

You don't have to upgrade to every new release immediately. It's OK to run slightly out of date versions of stuff. Want to know how I know? This very website, GitHub, is running on a version of Rails that is about 5 years old at this point. And that's OK.

wmfgerrit pushed a commit to wikimedia/mediawiki-extensions-Popups that referenced this issue Dec 14, 2018
redux       | 3.6.0 | 4.0.1
redux-thunk | 2.2.0 | 2.3.0

Upgrading these two dependencies caused the build size to increase from
from 12.2 KB to 12.4 KB (gzip) and so the bundlesize was adjusted
accordingly in our package.json.

Additionally, our linters flagged two rules that were then turned off in
.eslintrc.es5.json.

Looks like the changes in Redux were mostly cosmetic with much work
dedicated to typescript definitiions and dropping support of private
imports [1].

The changes to redux-thunk only involved changing typescript typings and
should not affect us. [2]

The upgrade was tested locally and did not appear to break anything.

[1] reduxjs/redux#1342 (comment)
[2] https://github.com/reduxjs/redux-thunk/releases/tag/v2.3.0

Bug: T209314
Change-Id: I35284793ca0c72914d9b9b2e7c28dd407bafd4d8
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