ImmutableJS dependency

[raulmatei]

Hi,

Is there any reason why ImmutableJS is in "dependencies" and not in "devDependencies" + "peerDependencies"?

I think there will be no issue with upgrading to the latest version and probably remove the Immutable from src/main export, and let developers manage the ImmutableJS dependency.

Any thoughts?

Raul.

[jordangarcia]

I've just put everything that goes into the compiled build under
dependencies

I'd be all for letting users switch out the version of Immutable. Any
thoughts on how to do this

On Saturday, February 13, 2016, Raul Matei notifications@github.com wrote:

Hi,

Is there any reason why ImmutableJS is in "dependencies" and not in
"devDependencies" + "peerDependencies"?

I think there will be no issue with upgrading to the latest version and
probably remove the Immutable from src/main export
https://github.com/optimizely/nuclear-js/blob/master/src/main.js#L13,
and let developers manage the ImmutableJS dependency.

Any thoughts?

Raul.

—
Reply to this email directly or view it on GitHub
#213.

[raulmatei]

I've had a look on this, externalizing ImmutableJS it's easy but it implies some other changes:

• webpack.config.js - add externals and list ImmutableJS see
• remove ImmutableJS from dependencies and add it into devDependencies and peerDependencies in package.json
• test if UMD build works, see here
• add lib/ directory that will end up as the main source for NPM users and change package.json main field to "main": "lib/main.js" - Babel can be used to transpile everything to ES5 - this is not really necessary since the UMD will work for CJS, AMD users too
• possibly move to Babel 6, and change how everything is exported from the main.js file, since imports are not destructuring and you can't do import { Store } from 'nuclear-js'; because Store it's a property of an object which is exported as the default - this will align the code base with ES2015 spec.
• fix failing tests, at this point just one: Reactor serialize/loadState when extending Reactor#serialize and Reactor#loadState should respect the extended methods - I assume this fails because of Babel and it reminds me of Inheriting from Reactor doesn't seems to work in v1.2.1 #191
• possibly move other CJS exports to ES6 - just to have consistency (not really needed)
• document changes
• document build process - right now only Grunt stuff is documented

I haven't tested anything related to Bower, and since I'm not using it in my projects I can't really say what should be changed to make sure it works, but I assume that ImmutableJS has to be added into dependencies on the developer's side.

Users that will use the UMD build with <script/> tags will have to include ImmutableJS before NuclearJS.

I worked on those changes on a NuclearJS fork. You can review the changes here: https://github.com/raulmatei/nuclear-js/commits/raulmatei/remove-immutable-js-dependency . I'll only start a Pull Request when everything is clear and all tests pass.

Hot-reloading example nuclear-js dependency points to my repository for now, I've just wanted to test without npm link.

Feel free to comment on this.

Raul.

[jordangarcia]

Awesome. I'll take a closer look this week

But it'd be happy to merge these improvements into Nucear

On Monday, February 15, 2016, Raul Matei notifications@github.com wrote:

I've had a look on this, externalizing ImmutableJS it's easy but it
implies some other changes:

• webpack.config.js - add externals and list ImmutableJS see
  https://webpack.github.io/docs/library-and-externals.html
• remove ImmutableJS from dependencies and add it into devDependencies
  and peerDependencies in package.json
• test if UMD build works, see here
  http://codepen.io/raulmatei/pen/QyYeoQ?editors=0010
• add lib/ directory that will end up as the main source for NPM users
  and change package.json main field to "main": "lib/main.js" - Babel
  can be used to transpile everything to ES5 - this is not really
  necessary since the UMD will works for CJS, AMD users too
• possibly move to Babel 6, and change how everything is exported from
  the main.js file, since imports are not destructuring and you can't do import
  { Store } from 'nuclear-js'; because Store it's a property of an
  object which is exported as the default
  https://github.com/optimizely/nuclear-js/blob/master/src/main.js#L10
• this will align the code base with ES2015 spec.
• fix failing tests, at this point just one: Reactor
  serialize/loadState when extending Reactor#serialize and Reactor#loadState
  should respect the extended methods - I assume this is failing because
  of Babel and reminds me of Inheriting from Reactor doesn't seems to work in v1.2.1 #191
  Inheriting from Reactor doesn't seems to work in v1.2.1 #191
• possibly move other CJS exports to ES6 - just to have consistency
  (not really needed)
• document changes
• document build process - right now only Grunt stuff is documented

I haven't tested anything related to Bower, and since I'm not using it in
my projects I can't really say what should be changed to make sure it
works, but I assume that ImmutableJS has to be added into dependencies on
the developer's side.

Users that will use the UMD build with <script/> tags will have to
include ImmutableJS before NuclearJS.

I worked on those changes on a NuclearJS fork. You can review the changes
here:
https://github.com/raulmatei/nuclear-js/commits/raulmatei/remove-immutable-js-dependency

Hot-reloading example nuclear-js dependency points to my repository for
now, I've just wanted to test without npm link.

Feel free to comment on this.

Raul.

—
Reply to this email directly or view it on GitHub
#213 (comment)
.

[raulmatei]

Sounds good.

In the mean time I've found the issue with the failing test. You've mentioned in #191 that Reactor cannot be called without new, but it looks that the exported class is wrapped in toFactory, which means that it should work if called without new.

Although I understand why toFactory was introduced it seems that it adds a lot of issues. Both Store and Reactor cannot be extended, see below:

And the code it's here even if it's not NuclearJS related, the behavior is the same.

Removing toFactory in reactor.js solves the failing test which I mentioned in my previous comment but it fails on Reactor should construct without 'new' test.

The question is, should Store & Reactor work without new in future versions? Is this really needed for backward compatibility? I mean, even if you upgrade to latest version, changing Store(...) to new Store(...) wouldn't be too hard, same for Reactor which in most cases is instantiated once.

Raul.

[jordangarcia]

Yeah this is really annoying. It's something Babel complains about when you
try to extend a factory method. Last I checked there was a Babel setting
that enabled you to still do this.

I agree that we probably want to move forward with the semantic new,
however I'm not willing to break semantic versioning for this.

We can include this and the new caching changes for a 2.0 version :)

On Monday, February 15, 2016, Raul Matei notifications@github.com wrote:

Sounds good.

In the mean time I've found the issue with the failing test. You've
mentioned in #191 #191
that Reactor cannot be called without new, but it looks that the exported
class is wrapped in toFactory, which means that it should work if called
without new.

Although I understand why toFactory was introduced it seems that it adds
a lot of issues. Both Store and Reactor cannot be extended, see below:
[image: image]
https://cloud.githubusercontent.com/assets/1718341/13060043/3e4d0c20-d437-11e5-9b28-84aa99df729d.png

And the code it's here
http://codepen.io/raulmatei/pen/eJXGEa?editors=0010 even if it's not
NuclearJS related, the behavior is the same.

Removing toFactory in reactor.js solves the failing test which I
mentioned in my previous comment but it fails on Reactor should construct
without 'new' test.

The question is, should Store & Reactor work without new in future
versions? Is this really needed for backward compatibility? I mean, even if
you upgrade to latest version, changing Store(...) to new Store(...)
wouldn't be too hard, same for Reactor which in most cases is instantiated
once.

Raul.

—
Reply to this email directly or view it on GitHub
#213 (comment)
.

[raulmatei]

I agree, those are breaking changes. On the other hand, I might try to externalize ImmutableJS with the current configuration (dev dependencies) but only if it makes it as a minor release, otherwise if it will only be released as a part of v2.0 it doesn't make sense to waste time with it and I'll continue on my fork with the changes listed above, including removing the factory wrapper.

Would it make sense to expose factories for creating stores and Reactor instances? E.g.:

import { createReactor, createStore, toImmutable } from 'nuclear-js';
import * as ActionTypes from './action-types';

const reactor = createReactor({ debug: true });

function increment(state, payload) {
  return state.set('count', state.get('count') + 1);
}

function decrement(state, payload) {
  return state.set('count', state.get('count') - 1);
}

const counter = createStore({
  getInitialState() {
     return toImmutable({ count: 0 })
  },

  initialize() {
    this.on(ActionTypes.INCREMENT, increment);
    this.on(ActionTypes.DECREMENT, decrement);
  }
});

// or createStore can take 2 arguments initialState (a plain JavaScript data type that 
// gets converted to a Immutable structure under the hood) and an object 
// containing all store handlers, e.g.:
//
// const INITIAL_STATE = { counter: 0 }
//
// createStore(INITIAL_STATE, {
//    [ActionTypes.INCREMENT]: increment,
//    [ActionTypes.DECREMENT]: decrement
// });

reactor.registerStores({ counter })

It solves the Store(...) and Reactor(...) if there are devs that don't like new, but to me it's something that I'd do on my side and I wouldn't expect it to be a part of the library.

Raul.

[raulmatei]

I think we can close this one.