cyclejs-community / redux-cycles

Bring functional reactive programming to Redux using Cycle.js
MIT License
745 stars 30 forks source link

0.3.0 - Export drivers to userland #28

Closed lmatteis closed 7 years ago

lmatteis commented 7 years ago

I've separated the two things into separate PRs. This only takes care of putting run in userland without Unified support.

goshacmd commented 7 years ago

Did you think about my idea of scoping the drivers under cycleMiddleware returned from createCycleMiddleware? We could probably even get rid of makeXDriver functions, instead exposing already-made drivers, which makes sense:

const cycleMiddleware = createCycleMiddleware();
...

ACTION: cycleMiddleware.actionDriver

So that, in the end, createCycleMiddleware acts as both:

lmatteis commented 7 years ago

@goshakkk ok but what would you pass it to applyMiddleware(...)? cycleMiddleware.middleware? Because applyMiddleware(...) wants a function, and in your example createCycleMiddleware() seems to return an object.

goshacmd commented 7 years ago

@lmatteis functions can have properties in js 😈

function createCycleMiddleware() {
  const middleware = () => { ... };
  middleware.actionDriver = ...;
  return middleware;
}

const cycleMiddleware = createCycleMiddleware();
// cycleMiddleware is a fn
cycleMiddleware.actionDriver
// but it also has other fields

There's nothing dirty about doing it like that.

Redux-saga does something similar as well: https://github.com/redux-saga/redux-saga#mainjs

lmatteis commented 7 years ago

Hrm not really happy about func props. You can't destructor them. I like the idea of having the drivers depend on the middleware though.

goshacmd commented 7 years ago

Fwiw, you can:

function a() {}
a.b = 5;
const { b } = a;
console.log(b) ;
// prints 5
lmatteis commented 7 years ago

@goshakkk ok makes sense. made changes. one thing is that i'm still exposing factories (makeStateDriver rather than stateDriver) because of this:

  cycleMiddleware.makeStateDriver = () => {
    const isSame = {};
    const getCurrent = store.getState;
    return function stateDriver() {

Where would getCurrent go otherwise (depends on store which is instantiated after applyMiddleware is called)?

goshacmd commented 7 years ago

@lmatteis I think makeStateDriver and makeActionDriver could be functions inside createCycleMiddleware, but what would be exported are not the factory functions themselves, but driver instances:

function createCycleMiddleware() {
  let store;

  function makeStateDriver() { ... }

  const middleware = () => { ... };

  middleware.stateDriver = makeStateDriver();
  return middleware;
}

Alternatively, we might not even need the factory functions at all, just assign the driver instances directly:

function createCycleMiddleware() {
  let store;

  const middleware = () => { ... };

  middleware.stateDriver = () => {
    const getCurrent = store.getState;
    return xs.create...;
  };
  return middleware;
}
lmatteis commented 7 years ago

@goshakkk can we rely on that though? The driver will be executed by Cycle.js, hence const getCurrent = store.getState; might get executed in ways we can't predict (multiple times perhaps). Also in terms of API, most cycle drivers are factories. What if we may need to pass arguments in the future?

lmatteis commented 7 years ago

@goshakkk i added a 1.0.0 commit with a CHANGELOG.md. Let me know what you think.

lmatteis commented 7 years ago

This PR should probably update the code in example/ with 1.0.0 changes. Problem is that it I can't make it depend on 1.0.0 because it isn't published yet. Should I just import ../ in the examples rather than redux-cycles from npm?

nevermind I can use npm local dependencies: "redux-cycles": "file:../",

lmatteis commented 7 years ago

Another small comment (sorry for the spam): makeStateDriver() needs to be called after run, otherwise store.getState is undefined. Any ideas about this?

goshacmd commented 7 years ago

@lmatteis just move that line to be inside stateDriver?

lmatteis commented 7 years ago

Ok this might be ready to merge. Will wait for the green light from you guys

lmatteis commented 7 years ago

Well it's a breaking change no? So we need to bump up the first number. 0.3 would mean new features and no breaking changes.

goshacmd commented 7 years ago

@lmatteis semver is pretty lax with 0.x's. 0.x means that a project is not yet ready for 1.0, but breaking changes can nevertheless naturally occur — because everything leading up to 1.0 is, in some way, "discovery" of the API, and use cases, and the bugs, and so on; v1.0 signifies the first "stable" / "reliable" version. So it's perfectly fine to make breaking changes and do a 0.3, 0.4, etc. while we're making and testing decisions.

I'd cut 1.0 when we have more people using redux-cycles in production and sharing their feedback, so we will be in position to say 1.0 is truly battle-tested.

More on semver: http://semver.org/#how-should-i-deal-with-revisions-in-the-0yz-initial-development-phase this obviously should be taken as a broad guideline, rather than a strict rule, but I do think this makes sense.

lmatteis commented 7 years ago

Right but most users that do npm i -s redux-cycles will have ^0.2.3 in their package.json. Meaning that if we release 0.3.0 instead of 1.0.0 it will break their code. I guess we can go for 0.3.0 since we're not widely used. But the API and code footprint is so small that it's hard to think we'd need to change the API sometimes soon - hence the fast bump to 1.0.0.

lmatteis commented 7 years ago

Jeez I didn't know there was so much disagreement with semver: https://github.com/dominictarr/semver-ftw/issues/2#issuecomment-13875242

lmatteis commented 7 years ago

Hold on, will npm not update ^0.2.3 to 0.3.0? I can't find the right docs, but if ^0.2.3 is essentially 0.2.x (because of the 0) then we can totally go for 0.3.0 instead of 1.0.0. I just don't want to break changes to users using ^0.2.3

EDIT: yup i tested it so npm is smart enough to interpret the leading 0. Ok se we can go with 0.3.0

lmatteis commented 7 years ago

Ok I think I took care of all comments. Waiting for your thumbsup @goshakkk