Adapters directory being passed to include-all breaks on v0.12.7 and above

[richdunajewski]

Sails version: 0.12.7+
Node version: 4.5.0
NPM version: 3.10.5
Operating system: Windows and Linux

---

We have a customized version of sails-mongo and sails-postgresql in our api/adapters directory. After upgrading to Sails v0.12.7 and above, we get the following error:

2016-11-09T19:39:57.962Z - error: error: A hook (`orm`) failed to load!
2016-11-09T19:39:58.234Z - error: error: Error: Attempting to flatten modules but duplicate key detected (`crossAdapter`).  Enable `keepDirectoryPath: true` to enable namepspacing based on hierarchy.
    at node_modules\sails\node_modules\include-all\lib\help-include-all-sync.js:217:52

After searching through the code, we do indeed find crossAdapter is referenced in each of the custom adapters. The below issue and subsequent fix that was made to v0.12.7 is what broke for us. I suspect that the include-all is being run on the entire api directory, which is causing the node_modules within each adapter to be included. When a collision is detected in crossAdapter.js, the error message is thrown.

If that's truly the case, it seems to me that Sails should exclude api/adapters from include-all as there should never be configuration files here, only adapters.

Please see #3846

[mikermcneil]

@richdunajewski Thanks for taking the time to write up a detailed report. I think, like you suggested, that the additional error checking added to include-all, combined with the fact that it's doing a recursive lookup, is what is causing the issue here. I'll look into it this evening. If, in the mean time, you could post your installed version of sails-hook-orm (a dependency of Sails) that'll help narrow it down. Thanks!

[mikermcneil]

@richdunajewski Thanks again for posting the link to the other issue. Talked about this with @sgress454 and he's working on a repro.

In the mean time, just to recap what we came up with for future reference:

• You're supposed to be able to make either api/adapters/FooAdapter.js-- but technically you can also do api/adapters/foo/index.js. Support for the latter was accidental and never explicitly intended, but it is definitely handy.
• In the ORM hook, we do https://github.com/balderdashy/sails-hook-orm/blob/for-sails-v0.12/lib/load-models-and-custom-adapters.js#L51-L75 which doesn't do much of anything extra beyond calling out to the moduleloader. So it's up to the moduleloader hook.
• Over in the moduleloader hook, currently in 0.12.x, we're looking for the FooAdapter.js style.
• There was a change here recently, but it was just a property name change.
• But as you can see, the Adapter is not optional and it's not looking for index.js.. so it's weird that it's loading the other files at all. (That's what Scott is looking into next.)

[sgress454]

@richdunajewski Alright, this should be addressed in the latest code. With the latest updates, I'm not sure your app will lift without any changes, because it seems likely that you were organizing your custom adapters in a way we never intended to support (the documentation on custom adapters was admittedly thin, and @mikermcneil is working on it as we speak). The way it works now (and how it was always intended) was that for a custom adapter "Xyz", you can either:

1. Put a XyzAdapter.js file at the top level (e.g. api/adapters/XyzAdapter.js), or
2. Put an index.js in an api/adapters/xyz folder

So you may have to update your folder structure / filenames slightly to account for this. But the crossAdapter conflict problem should be gone. If you wouldn't mind checking out master (doing npm install balderdashy/sails#0.12 should do the trick) and confirming for us, we can publish a patch. Thanks!

[richdunajewski]

Thanks @mikermcneil and @sgress454 for the quick attention on this. I've installed Sails from master, and the error about crossAdapter is now gone, however there's now just a simple ORM failed to load error.

verbose: Setting Node environment...
verbose: In this Sails app, detected special code languages: 0 []
verbose: moduleloader hook loaded successfully.
verbose: Loading app config...
verbose: userconfig hook loaded successfully.
verbose: Exposing global variables... (you can customize/disable this by modifying the properties in `sails.config.globals`.  Set it to `false` to disable all globals.)
verbose: Loading user hooks...
verbose: Found hook: `orm` in `node_modules/`.  Overriding core hook w/ the same identity...
verbose: Found hook: `sockets` in `node_modules/`.  Overriding core hook w/ the same identity...
verbose: userhooks hook loaded successfully.
verbose: Loading runtime custom response definitions...
verbose: Loading app services...
error: error: A hook (`orm`) failed to load!

I then changed sails-mongoAdapter.js and sails-postgresqlAdapter.js both to index.js and get the same ORM error as before. It seems as though the adapters aren't loading properly now.

Aside: Is there a clearer way to see what causes the ORM error under the covers? I find it difficult to troubleshoot ORM errors in general, particularly when there are many different connections in use. It's hard to tell which connection is the culprit.

Just to clarify on my folder structure under api/adapters, I have waterline adapters taken from npm that I've modified (but can't simply fork them in GitHub, or it would be much easier to just install them normally):

I'll continue to troubleshoot what's going on internal to Sails and report back if I find anything interesting.

[richdunajewski]

Ah, the curse of posting publicly.

Immediately after posting the above I found the culprit for the ORM error. I had another adapter (not pictured) that was written by us rather than installed from npm, that still followed the sails-fooAdapter.js naming convention (although, it wasn't nested with node modules). Changing that file to index.js as well allowed Sails to start.

It looks like the change by @sgress454 is working beautifully. Thank you both!

[mikermcneil]

It looks like the change by @sgress454 is working beautifully. Thank you both!

@richdunajewski 👍

Aside: Is there a clearer way to see what causes the ORM error under the covers? I find it difficult to troubleshoot ORM errors in general, particularly when there are many different connections in use. It's hard to tell which connection is the culprit.

Best I can suggest there right now is using --silly, but that's definitely not always the answer. We've done a few fixes to sails-hook-orm for common errors recently, but for some config problems, you still have to follow the stack trace, unfortunately. Improving the debugging experience (and adding internal assertions throughout Sails and Waterline in general) is something we're working to on in Sails v1/Waterline 0.13.

[mikermcneil]

@sgress454 did you already take care of 4495a16 on master, or should I rebase it?

[sgress454]

@mikermcneil Took care of it on master, but 0.12 still needs to be published

[mikermcneil]

Published! 0.12.10