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

Remove ImmutableJS dependency #214

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

raulmatei
Copy link
Contributor

As discussed in #213 I was able to keep the current codebase and only remove Immutable dependency. Nothing else is touched, so everything should work as expected. Distribution files are not updated, so if you want to test please make sure you recompile them with webpack. Do not forget to install ImmutableJS as a dependecy in your project's package.json.

@andrei-cacio
Copy link
Contributor

+1

@jordangarcia
Copy link
Contributor

What would be the ideal way to use this if you do not want to opt in to a bundled ImmutableJS Nuclear build.

It seems improper to break backwards compatibility if you are already relying on ImmutableJS being bundled.

@raulmatei would you be okay with having to require a sepcific src file or dist file that isn't the package.json main entrypoint?

@balloob
Copy link
Contributor

balloob commented Aug 24, 2016

Starting with npm3 this has actually become a breaking change:

With the introduction of NPM3, peerDependencies behaves differently. It no longer actually installs the peerDependency packages. Instead, it just checks at the end of install to see that they are resolvable, and prints a warning if they are not.
source

As the PR shows, it doesn't require any changes to the code. That however does mean that if you want to avoid a breaking change, you cannot easily solve it by providing a different entry point.

One reason why I think it is a good move is that it would make it easier to include NuclearJS in projects that already depend on ImmutableJS without having to adopt the version that comes with NuclearJS. This for example would also remove the push for NuclearJS to release a new version every time ImmutableJS releases a new version.

@iansinnott
Copy link

What about a major version bump?

@raulmatei
Copy link
Contributor Author

raulmatei commented Aug 25, 2016

@jordangarcia - for time being, having a second build without ImmutableJS would be a way to solve the backwards compatibility issue: import Store from 'nuclearjs/a-good-name-for-the-non-immutable-version', but personally I was looking more after using latest versions of ImmutableJS and not really removing it from NuclearJS bundle, even though I think that the best solution is to keep it outside the bundle because maybe there will be people using an older version and they don't want to upgrade ImmutableJS but want latest features of NuclearJS.

@balloob - yes, I think is a breaking change too and it would probably mean a major version bump as @iansinnott said.

@jordangarcia
Copy link
Contributor

I'll work on getting a major version out this week. I just want to make sure it's natural for people to include ImmutableJS with going through too much trouble.

@jordangarcia jordangarcia added this to the 2.0 milestone Sep 21, 2016
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.

5 participants