Allow usage in linked modules

[karlhorky]

request-local-storage throws RLS() access outside of request! when used in both a parent module and a linked module.

This PR works around the issue by adding a key to the global namespace and exporting that same key if it already exists (singleton style).

Example error / repo: karlhorky/react-server-bike-share-example#2

[gigabo]

This seems a little risky. If I have two packages that depend on different versions of RequestLocalStorage then one or the other of them is in for a surprise. This package has been pretty stable for a while now, but if we ever find and fix a bug or add a useful feature then this patch could lead to maddening behavior.

Really RequestLocalStorage, like React, needs to be a singleton. Currently it throws an explicit error if multiple copies are used. This patch would make it succeed with undefined behavior.

I appreciate the convenience this provides, though. What about a version check? If the global copy is the same version as the currently initializing copy then return a reference to the global. Otherwise throw a require time error?

[doug-wade]

Really RequestLocalStorage, like React, needs to be a singleton.

So why not check that instead? Something like

const path = require('path');
const matches = Object.keys(require.cache).filter(p => p.includes('request-local-storage');
if (matches.length > 1 || matches[0] !== path.join(__dirname, __filename)) {
  throw new Error('request-local-storage must be a singleton');
}

[gigabo]

Does that address the issue @karlhorky is having?

I had thought he was in a situation where two packages each had their own copy of RLS.

[karlhorky]

Yeah, I'm not sure that @doug-wade's solution alone would address the original problem.

However, maybe we could use the require.cache along with the idea of versioning? I'll update the PR.

[karlhorky]

@gigabo @doug-wade ok, I've added versioning to the singleton (via gulp build step). How does this look?

[gigabo]

Yeah, this looks safer.

The build-time replacement of REQUEST_LOCAL_STORAGE_VERSION is a neat trick, but the cost of just looking it up at run-time isn't really that great. Might be a case where simplicity is worth it. Either way eslint is choking on it, so you'll need to do something to get ci passing. :)

[karlhorky]

Ah sorry, I need to remember to check the tests before pushing 😛 I've added REQUEST_LOCAL_STORAGE_VERSION as a global in the .eslintrc config file.

Might be a case where simplicity is worth it.

I'm not sure I understood this suggestion, which led me to the quick fix instead of doing more.

If you meant that we should have a version number directly in this src/index.js file, we could do that, but it could be potentially fragile (if someone forgets to update it while releasing a new version). Of course we could always just write a test to make sure that it's the same in the package.json and in the src/index.js, but that seems like it could be a bit roundabout...

[gigabo]

I'm not sure I understood this suggestion

I just meant we could check require("./package.json").version at run-time. If it were at the module level the require cache would mean we're only checking once, so it wouldn't be a huge cost.

Oh well, though, this seems fine to me. 👍

[karlhorky]

we could check require("./package.json").version at run-time

Ah yeah this was my first idea, but I read from user Pathogen that this could be a security concern: http://stackoverflow.com/a/10855054/1268612

[gigabo]

@karlhorky Thanks for the link. Not so concerned about the security implications of exposing RLS's package.json (it's open source), but the require would increase bundle size, which would be a bummer. Hadn't considered that.

Nice improvement. 👍

[karlhorky]

Thanks!