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

Store management with createStore #71

Merged
merged 15 commits into from
Jan 2, 2017
Merged

Store management with createStore #71

merged 15 commits into from
Jan 2, 2017

Conversation

fahad19
Copy link
Member

@fahad19 fahad19 commented Dec 28, 2016

What's done

  • New createStore function
    • Used internally for handling Stores for App instances
  • Dropped redux
  • Fully backwards compatible with deprecated methods
  • Full test coverage
  • Minified/gzipped dist file down to ~7kB from ~13kB

Preview

Both examples in the repo working, and here's a screenshot for Multiple Widgets:

screen shot 2016-12-28 at 15 52 08

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 66548ea on store into e61aa26 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 70acc5c on store into 1ccbc12 on master.

.scan((previousState, action) => {
let updatedState;
const d = new Date();
const prettyDate = `${d.getHours()}:${d.getMinutes()}:${d.getSeconds()}.${d.getMilliseconds()}`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember, you don't have zero-padded integers.

}

// logger in non-production mode only
if (process.env.NODE_ENV !== 'production') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this logic should go to the logger itself, or better yet, whenever you create an instance of the logger you could specify the environment(s) in which it should work?

Why? Because of this: nodejs/node#3104

Another way would be to cache that value at the beginning of the module. :)

Copy link
Member Author

@fahad19 fahad19 Dec 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mAiNiNfEcTiOn: I kept it like it was working before. The main reason is because of our production bundles.

When NODE_ENV === production, the full code-block inside this if would not end up in the bundle processed by Webpack (in our case, it is vendors.js file).

If we want to check it in runtime instead, it means the logger specific code would have to be in the bundle all the time. Do we want that in this PR?

Please correct me if I understood your comment wrong.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, clear then.

return this.cachedState;
}

dispatch = (action) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this action the String (e.g. 'MY_ACTION') or the Object passed with the type?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mAiNiNfEcTiOn: sorry for the confusion. I should add some docblocks soon.

Store.dispatch can accept:

  • a payload, like: {type: 'DO_SOMETHING'}, or
  • a function, like:
    function (dispatch, getState, { app }) {
      aTimeConsumingPromise()
        .then(function () {
          dispatch({ type: 'DO_SOMETHING' });
        });
    }

It maintains the sync/async actions, just like how we support them now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

;) Sure... good good. That's ok then :) 👍

@markvincze
Copy link
Contributor

markvincze commented Dec 29, 2016

The couple of deleted test files, like appendAction, and async: why aren those not needed anymore?

And just to make sure I understand correctly: instead of depending on redux, you implemented a small part of its functionality in createStore.js?

@fahad19
Copy link
Member Author

fahad19 commented Dec 29, 2016

@markvincze: they are deleted because they were middlewares for Redux, now they are supported directly via createStore function, in the form of passed options.

See these options in createStore.js:

  • thunkArgument: for deleted async.js middleware
  • appendAction: for deleted appendAction.js middleware
  • enableLogger: for deleted logger.js middleware

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling d72a158 on store into 1ccbc12 on master.

@@ -18,7 +18,15 @@ class BaseStore {
.scan((previousState, action) => {
let updatedState;
const d = new Date();
const prettyDate = `${d.getHours()}:${d.getMinutes()}:${d.getSeconds()}.${d.getMilliseconds()}`;
const prettyDate = [
_.padStart(d.getHours(), 2, 0),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-.-' One liners...:

((str) => '0'.repeat(Math.abs(str.length - 2)) + str)(d.getHours())...

But what do I know? 😄

@fahad19 fahad19 added the minor label Jan 2, 2017
@fahad19 fahad19 merged commit 8900863 into master Jan 2, 2017
@fahad19 fahad19 deleted the store branch January 2, 2017 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants