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

Always serializes to Immutable.Map if multiple immutable libs are used #32

Open
nkbt opened this issue Feb 17, 2017 · 9 comments
Open

Comments

@nkbt
Copy link

nkbt commented Feb 17, 2017

We've spent quite some time debugging one very strange issue. The effect was in Immutable.List being serialized and later de-serialized into Immutable.Map.

I will try to explain the problem as short as possible and suggest a solution at the end.

Summary

instanceof comparisons of transit-js fails to compare Immutable types if we have multiple immutable libraries used.

Explanation

We are using Lerna monorepo and all internal packages there are symlinked (the same effect can happen if you use npm link without doing work in monorepo at all)

Bootstrapped working project structure:

packages
  app
    node_modules
      immutable
      ->lib
  lib
    node_modules
      immutable

As you can see symlinked lib will also have its own immutable like:

packages
  app
    node_modules
      immutable
      ->lib
        node_modules
          immutable

  lib
    node_modules
      immutable

As soon as we export anything Immutable in lib and use it in app, trying to serialize the state would result in fallback to the default case of transit-immutable-js:

"default", transit.makeWriteHandler({
  tag: function() {
    return 'iM';
  }

The reason for that is transit-js tries to do a match by using instanceof and lib->Immutable.List does not match app->Immutable.List

Possible solution

Instead of passing a mapping of Immutable.X to transit-js always use default case and use native ImmutableJS methods like Immutable.List.isList(obj) to do matching.

We checked that in the app's code and Immutable.List.isList(obj) works perfectly no matter what immutable is used there.

We did not check this on transit-immutable-js code yet. Need to see if it even possible (would be if transit-js somehow gives obj back to tag function of writeHandler

@glenjamin
Copy link
Owner

I think this should work, that's an interesting pitfall with lerna and link to be aware of when things use instanceof.

Would take a PR for this.

@nkbt
Copy link
Author

nkbt commented Feb 20, 2017

I'll see what I can do about it.
Currently I sort of fixed it for our packages by creating @namespace/immutable and replaced all require('immutable') with it to ensure the only immutable instance is ever used.

But that is a short-term approach and we need better solution for sure.

btw this is fairly common issue as far as I can tell. npm link stopped being that useful as soon as we moved to compiled javascript and some libraries. So we all should consider this sort of issues will be appearing from time to time.

@giautm
Copy link

giautm commented Feb 28, 2017

Same issues here, waiting PR for a fix.

@nkbt
Copy link
Author

nkbt commented Mar 29, 2017

Unfortunately this problem is not specific to transit-immutable-js only. Several other "singlton" libraries out there stop working properly with symlinks (done by npm link, lerna bootstrap, etc). Another example would be react-redux. And some other libs (effectively any lib that does instanceof checks internally).

Extracting own common lib can get you only this far. Until you use some 3rd-party that depend on one of your "proxied" library.

Our final solution for this problem looks this way.

// Server-side
const jsHook = require('babel-register');
jsHook({
  // ...
  plugins: [
    [require.resolve('babel-plugin-module-resolver'), {
      root: [path.join(process.cwd(), 'node_modules'), './node_modules']
    }]
  ]
});

// Client-side, webpack.config.js
module.exports = {
  // ...
  resolve: {
    modules: [
      path.join(process.cwd(), 'node_modules'),
      'node_modules'
    ]
  }
};

NodeJS default behaviour is to search for modules depth-first (so it will follow all symlinks until reaches the deepest and closes library that is required.

Changes above flip this upside-down and node would look for a dependency first at entry point and only then will fall back to default behaviour.

It works well for all our projects now.

Note that require.resolve for babel plugins is required. If we don't do that - each other symlinked dependency would have to have those babel plugins installed for them. So instead of relative link we give it full path for babel plugin and it needs to be installed only for the parent project.

We've spent quite a bit of time making these things work and ended up with this solution. Pretty sure I've got couple of grey hairs now added...

@nkbt
Copy link
Author

nkbt commented Mar 29, 2017

Apparently react-redux solved the issue on their side now in reduxjs/react-redux#628

@glenjamin
Copy link
Owner

Yep, in general I try and avoid instanceof checks, but transit doesn't give me much choice in this case.

@cbjs
Copy link

cbjs commented May 3, 2017

any news about this issue?

@nkbt
Copy link
Author

nkbt commented May 3, 2017

I actually wrote my owt transit library about 200 lines long that replaces both this one and whole transit-js. It is not hard especially if you are not trying to solve a one-fits-all usecase (not opensourse since we don't want to support generic cases and just keep it focused on our own data sttuctures)

I need to find some time soon to backport part of its code here to solve this issue. It is actually pretty straightforward

@jeffberry
Copy link

I had this same problem, I ended up figuring out it was because react native had it's own instance of immutable installed in node_modules/react-native/node_modules/immutable. I was using a different version of Immutable in my app's package.json; once I switched that version to the same version react-native requires (~3.7.6) the second installation in the react-native folder went away, and my issues were fixed. Thought this might be helpful to some.

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

No branches or pull requests

5 participants