Skip to content

Add seamless support for ImmutableJS structures in select() path #160

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

Merged
merged 4 commits into from
Jul 18, 2016
Merged

Conversation

clbond
Copy link
Contributor

@clbond clbond commented Jul 17, 2016

This change allows you to use syntax like @select(['foo', 'bar', 'baz']) where either 'foo' or 'bar' are ImmutableJS structures inside of your Redux state.

@SethDavenport
Copy link
Member

Hey @clbond this is an interesting idea. However I have some reservations about making immutable a dependency of ng2-redux.

Not everyone who uses ng2-redux wants to use or even depend on immutableJS.

It's also worth noting that you can use @select(['path']) with Immutable if you define your reducer data with Immutable.Record instead of Map. See https://github.com/rangle/angular2-redux-starter/pull/217 for an example of what I'm talking about.

That said, there's still some merit to a general approach for Iterables like this - perhaps there's a way to put it in its own package somehow?

@clbond
Copy link
Contributor Author

clbond commented Jul 17, 2016

Good point. Pushed a change that makes immutable an optional dependency. If the project is not using immutablejs, then neither will ng2-redux; if it is using immutablejs, then ng2-redux will use it.

@SethDavenport
Copy link
Member

Might be worth adding a couple of unit tests? For Map and List?

@e-schultz what's your take in this approach?

@clbond
Copy link
Contributor Author

clbond commented Jul 18, 2016

@SethDavenport Good call; added some unit tests related to optional immutable support (without depending upon immutablejs itself in order to write the tests, just creating mock objects that mimic immutablejs structures).

@clbond
Copy link
Contributor Author

clbond commented Jul 18, 2016

I personally think it is important to include this optional support, especially since we are able to do it without introducing a dependency upon immutable and since we use ng2-redux together with immutable in all of our starter projects. I tested the changes on a project that uses immutable and one that does not, and it worked as expected. The project that had the immutable dependency got automatic ng2-redux support for Immutable structures. The one that did not depend on immutable just got the same old behaviour.

@e-schultz
Copy link
Member

Overall I like the idea, especially with how common it is to use redux with immutable

@clbond
Copy link
Contributor Author

clbond commented Jul 18, 2016

I would tend to agree that the way that I implemented the optional immutable stuff is a little sketchy, having that try-catch block around the require() statement. But in looking around at how other people have confronted this issue of optional dependencies, they all used the same method. There is even an npm package called "optional" (https://www.npmjs.com/package/optional) which allows this syntax:

const dep = optional('dependency');

And the way that it implements that functionality is using the same try-catch-require block:

module.exports = function(module, options){
  try{
    return require(module);
  } catch(err){}
  return null;
};

So I think this is a perfectly safe way of saying "if the project that is being compiled has an existing immutablejs dependency, then use it." And we don't have to add immutable to our package.json under devDependencies or peerDependencies.

@SethDavenport
Copy link
Member

Works for me then. :shipit:

@SethDavenport SethDavenport merged commit c19f87d into angular-redux:master Jul 18, 2016
e-schultz added a commit that referenced this pull request Jul 21, 2016
* 3.3.0

* Features
 * DevToolsExtension - convience wrapper for dev tools (#115)
 * Select - seamless support for ImmutableJS (#160)

* Fixes
 * Able to use `@select` in services
 * Behavior of `select` with chained dispatches, (fixes #149, #153)
@e-schultz e-schultz mentioned this pull request Jul 21, 2016
e-schultz added a commit that referenced this pull request Jul 21, 2016
* 3.3.0

* Features
 * DevToolsExtension - convience wrapper for dev tools (#115)
 * Select - seamless support for ImmutableJS (#160)

* Fixes
 * Able to use `@select` in services
 * Behavior of `select` with chained dispatches, (fixes #149, #153)
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.

3 participants