Sails include-all breaks node's module cache (require.cache)

[mactyr]

Node version: 14.16.0 Sails version (sails): 1.4.2 ORM hook version (sails-hook-orm): N/A Sockets hook version (sails-hook-sockets): N/A Organics hook version (sails-hook-organics): N/A Grunt hook version (sails-hook-grunt): N/A Uploads hook version (sails-hook-uploads): N/A DB adapter & version (e.g. sails-mysql@5.55.5): N/A Skipper adapter & version (e.g. skipper-s3@5.55.5): N/A

---

In my sails application's config/local.js, I require some shared configuration from outside my sails directory (but within my broader code base) to help determine my sails configuration. E.g., in local.js I have:

const config = require('../../shared-dir').config

shared-dir/ has an index.js file that exposes a number of modules within that directory including config. I then use config to help determine my sails configuration.

shared-dir/ also contains a file called (for example) shared-object.js, which is also exported by shared-dir/index.js as shared-object. My code base is expecting shared-object to always be the same object regardless of where or when it is required. That is, I'm relying on the normal behavior of node's require cache to ensure that every time I require ../../shared-dir/shared-object.js I get the same object. I am not expecting shared-object.js to be re-evaluated from scratch every time I require it, resulting in a new instance of the object, because that's not how node works.

However, I've now discovered that by default, sails' include-all deletes from the require cache anything that it includes and anything required by those files, and so on! This results in different parts of my application having independent, unrelated copies of shared-object depending on whether they were required before or after a given use of include-all. This caused a bug in my application that was quite difficult to track down because the root cause seemed contrary to everything the node documentation/community says about how the require cache works.

A comment in help-include-all-sync.js describes this default deletion behavior as "sane defaults". I don't know enough about the history/goals of that package to understand why cache deletion was chosen as the default, but to me changing core platform mechanics without loud and clear documentation does not seem... sane. :shrug:

In the example outlined above, I should be able to work around the problem by changing

const config = require('../../shared-dir').config

to

const config = require('../../shared-dir/config')

so that the rest of the directory (including shared-object) doesn't get swept up in include-all's deletion routine, but I will probably have to hunt down other instances of this as well, and the deletion behavior remains a potential source of bugs for anyone who isn't aware of the need to carefully consider the way they scope their requires within sails.

[sailsbot]

@mactyr Thanks for posting! We'll take a look as soon as possible.

In the mean time, there are a few ways you can help speed things along:

• look for a workaround. (Even if it's just temporary, sharing your solution can save someone else a lot of time and effort.)
• tell us why this issue is important to you and your team. What are you trying to accomplish? (Submissions with a little bit of human context tend to be easier to understand and faster to resolve.)
• make sure you've provided clear instructions on how to reproduce the bug from a clean install.
• double-check that you've provided all of the requested version and dependency information. (Some of this info might seem irrelevant at first, like which database adapter you're using, but we ask that you include it anyway. Oftentimes an issue is caused by a confluence of unexpected factors, and it can save everybody a ton of time to know all the details up front.)
• read the code of conduct.
• if appropriate, ask your business to sponsor your issue. (Open source is our passion, and our core maintainers volunteer many of their nights and weekends working on Sails. But you only get so many nights and weekends in life, and stuff gets done a lot faster when you can work on it during normal daylight hours.)
• let us know if you are using a 3rd party plugin; whether that's a database adapter, a non-standard view engine, or any other dependency maintained by someone other than our core team. (Besides the name of the 3rd party package, it helps to include the exact version you're using. If you're unsure, check out this list of all the core packages we maintain.)

  ---

Please remember: never post in a public forum if you believe you've found a genuine security vulnerability. Instead, disclose it responsibly.

For help with questions about Sails, click here.

[eashaw]

Hey @mactyr, thank you for all the info. Do you have any thoughts about where we could document this behavior, that would have made it easier to debug?

[mactyr]

@eashaw, fair question. Really what I think is that sails should not remove anything from the require cache; then there's no surprising behavior to document!

So, first I'd ask, is it absolutely necessary that the cache be bypassed/edited in the contexts where sails uses include-all in the first place? Or is it just default behavior that's never been questioned because no one's noticed it causing problems? I don't know enough about sails internals to answer that myself.

Assuming that the cache bypassing is strictly necessary, you might look at using stealthy-require, which I noticed being used by another package I import. stealthy-require temporarily empties the require cache in order to bypass it, but then restores the cache after it's done with whatever it's been asked to require, so the cache behaves more as expected for the rest of the application. (Though there is still some possibility for breakage in that any module imported by include-all would be a different copy than the one the rest of the application is using.)

If the behavior can't be changed (or until it's changed) I'd suggest documenting it somewhere up front in the configuration section (e.g. https://sailsjs.com/documentation/reference/configuration) since that's where I noticed the problem manifesting. You could note that any module previously required by the application prior to loading the configuration, and that is also required by a configuration file, will be swapped out for a re-initialized copy after the configuration loads. There may be other places where include-all is used as well, but that's the one that burned me.

Also, my hope is that just having this issue on record will help! When I googled terms like "sails require cache" nothing helpful came up, but hopefully it will now for others. :slightly_smiling_face:

[eashaw]

Hey @mactyr, I'm sitting with @mikermcneil and going over the history of this decision.

Here's what I learned:

• Module loader was added in 2013 in Sails 0.9 as part of the new (at that time) hooks system. Initially, we did not clear the require cache because we had the same thoughts about this (and it's the default behavior)
• By 2014, various folks in the community have built hooks at this point, despite a lack of documentation for manipulating the internals of the Sails framework post lift. One popular community hook that had been built (in many different ways by different people) was auto-reload functionality for the server (i.e., making it so you don't have to kill the server and lift again)
• Around that time (or later), Mike changed module loader to start clearing the require cache to fix issues that were common in apps using hooks that manipulated things like routes and actions at runtime. This went through a few follow-up passes to get to a stable state for all of the different ways people were working with the framework.

We're open to having this be configurable, but it would involve testing and feedback from people in the community that use an implementation of auto-reload.

Thank you for all the detail and for prompting us to have a discussion!

[mactyr]

Ok, thanks for looking into it. A couple of clarifying questions:

1. Is "module loader" the same thing as "include-all"? Because the problem I noticed seems to be specific to include-all.
2. Your third bullet makes it sound like the last change was to stop clearing the require cache, but the problem that I'm reporting is that sails (more specifically, include-all) is clearing the require cache. Was the "stop" just supposed to be a "start", or am I missing something?

[eashaw]

1. Module loader is a hook in Sails
2. Yes, the stop was supposed to be start(edited, thank you)

[yuvalherziger]

I'm experiencing a similar issue. In my case, it essentially means that without intrusive, counter-intuitive patches, I can't leverage the singleton nature of node modules if Sails's objects require them.