[Proposal] Ability to run code on load before doing _anything_ else (re: Add user configurable language support)

[internalfx]

@tjwebb @sgress454 @loicsaintroch @mikermcneil

This PR keeps the current functionality the same for existing users, but allows them to override the default supported languages in moduleloader

the .sailsrc can now have a languages key added that will replace the default supported languages array.

This brings us at least closer to what we wanted all along, It still does not allow the use of hooks, but its already been discussed why we need a whole rewrite to fix that.

This is a good stop gap I think.

Thoughts?

[mikermcneil]

Super late response from me here @internalfx, apologies.

I think this is a great solution, and the way it is set up is extensible for any future use cases that have specific needs -- e.g. in the future we could, if necessary, allow an asynchronous function to be provided in lieu of the require key, for anyone working with a custom transpiler/precompiler that doesn't support standard node requires for some reason-- or if there are ever issues with global npm deps-- it's always worried me a bit that we rely on a global coffeescript dep. for folks using coffee on the backend. That said, specifying functions in the .sailsrc file is a no-go currently, since it's parsed as JSON (this is done in dominiktarr's rc), so people who need this would have to specify this info in their app.js file, and they wouldn't be able to start sails with sails lift (which sucks). Eventually, I'd be 100% down to add support for the .sailsrc file being specified as a normal node module (exporting a dictionary of config), but I doubt dominick would want that in core. Anyways, in short I don't think we could quite use functions for this yet, so the approach you've suggested looks great.

The only tweak I'd suggest is that, instead of languages, can we think of a different key for this? It'd be easy, esp. for new users, to confuse this w/ i18n. What about extensions (for consistency w/ node core)?

Other than that, I think your solution proposed here is great for Sails going forward, and I'd like to try and get it in the 0.12 release if possible. If you wouldn't mind sending a PR to the 0.12 branch of sails-docs that adds extensions.md to the config reference docs, then we can go ahead and get this in asap.

For the reference docs, please make sure to:

• prominently mention that extensions must be included in the .sailsrc file or provided programmatically (and that otherwise it is too late and other hooks have already to load)
• clarify that extensions is an array of dictionaries (many of the other top-level config keys are dictionaries)
• use a table formatted like this to describe the properties of each extension in the array.

It would be great to have a deeper explanation of coffeescript usage in Sails in our conceptual docs as well, but that is not critical (and it takes more work) so that's definitely not a blocker for getting this merged in asap. Thanks a lot!

P.S. To anyone else reading this: if you have the time, it would be really helpful if you could add a couple of tests. I realize this might take some effort (at least personally, I'm not sure offhand the best way to test this-- might be easy, might not-- you never know until you dive in). This doesn't necessarily need to happen before we land this enhancement in core, since our contribution guidelines have changed a number of times recently, and today there aren't tests for the current support for require extensions anyway. But that cycle has to stop somewhere-- one way or another, tests for this feature will need to get written.

Side note:
These kinds of untested changes tend to stack up, and before you know it, otherwise-elegant and innocuous-seeming changes we make in the future end up breaking untested features. This affects users that are relying on untested features that were added in PRs, who end up creating issues and adding to the burden of Sails contributors. Also, whenever there is doubt, untested patches sent in PRs are always the first suspects, and sometimes even end up as scapegoats down the road; i.e. getting deprecated and ripped out. But most of all, the merging of untested features can be demotivating for everyone working on Sails, especially the core team. While we're used to staying on top of the changes we make ourselves (mostly since we talk to each other every day), it's almost impossible to keep up with all of the changes that might have been merged across the various repos which make up Sails core. The only way to solve this to ask for all code changes to come with tests and to update any relevant documentation.

Anyway, this isn't meant as a guilt trip; I just feel it's important to provide a little more explanation of where we're coming from on our testing philosophy and some of the recent updates to the Sails project contribution guide. This is definitely a gradual process, but since stepping up our enforcement of these rules, I've been really pleased and appreciative of everyone's positive attitude and willingness to help. Thank you!

[internalfx]

in the future we could, if necessary, allow an asynchronous function to be provided in lieu of the require key, for anyone working with a custom transpiler/precompiler that doesn't support standard node requires for some reason

@mikermcneil That's a great idea....

What if we just quit messing around and created a preload.js file that sailscore requires...

And users can do whatever they want in it. I didn't even think of that initially.

[internalfx]

heh....maybe prelift.js 😄

[internalfx]

This has some elegant simplicity to it....

You want to run require('coffee-script/register') before lift...be my guest!

I like your idea better than mine....

[mikermcneil]

heh....maybe prelift.js

:)

What if we just quit messing around and created a preload.js file that sailscore requires...

I think that would work nicely-- i.e. an optional JavaScript file that exports a function that Sails core will look for in the current repo (if it can't require it, no bigs, if it's the wrong type, we throw an error explaining what it's supposed to look like). I would suggest .prelift.js -- only thing here is that it's technically "preload" too since there's a vaguely arbitrary distinction between calling .load and .lift programmatically-- it's trivial, but I'd hate to confuse anyone.

On the other hand, for consistency with .sailsrc and our configuration structure, I think maybe a hidden file like .sailsrc.js would give us the most flexibility. Instead of exporting a function, it'd export a dictionary (just like the JSON in .sailsrc). This would allow you the flexibility to do whatever you want before Sails starts loading other config and/or hooks (i.e. at the top of the file), plus you'd be able to send in functions to sails.config right from the get-go by exporting them. How's that sound?

[mikermcneil]

So I think there are two parts to this that may or may not actually need to be the same feature:

1. Add the ability to run an asynchronous function before sails begins loading without having to hack your app.js file (and therefore enabling it to work when using sails lift from the command-line)

As discussed, a sails.config.beforeLift sort of thing (although like I said before, we'd want to call it beforeLoad to avoid confusion-- since it technically would fire with sails.load() too). This seems totally doable-- either as a simple check in Sails core, or potentially using the strategy below.

Honestly, the strategy of looking for a special beforeLoad.js file is probably the simplest way to go here.

2. improve the ability to specify custom configuration (other than strings) at the earliest stage of Sails loading without having to hack your app.js file (and therefore enabling it to work when using sails lift from the command-line)

Some equivalent to the .sailsrc file, but that also supports "wet" or non-JSON-serializable data (e.g. functions). Alternately, we could modify rc to support hydrating functions within nested config from strings (see https://github.com/node-machine/rttc#hydratevalue-typeschemaundefined). See also https://github.com/node-machine/rttc#parsehumanstringfromhuman-typeschemaundefined-unsafemodefalse.

The big advantage to the second approach is that it would also mean being able to specify config like the bootstrap function as an environment variable (plus even basic things like numbers, booleans, and nested dictionaries and arrays). Practically, this last case doesn't come up until you're in production and you're using env variables extensively already, but seeing as that's generally the recommended strategy, it makes sense to allow for more flexibility there so we make environment variable names conventional (e.g. you'd be able to do sails_http__middleware__order='["foo", "bar"]').

To be specific, this approach would allow you to specify any JSON-compatible value, as well as potentially stringified functions.

[internalfx]

I'm thinking we should dump this PR, and just add the mentioned beforeLoad.js. It's probably such an easy fix...like 20 minutes...maybe.

And it solves a whole bunch of problems for users.

[mikermcneil]

@internalfx agreed-- and it would solve https://github.com/balderdashy/sails/blob/master/lib/hooks/moduleloader/index.js#L145

(although on a separate note, I do like the auto-detection.... and I get why it's sync right now-- just need to figure out a way to make it faster / backgrounded, and also to pull it out of core. E.g. hooks can already register their own route target syntax: since the moduleloader exposes acceptable file extensions as a config, it seems like it wouldn't be too hard to allow a hook to get in the middle and provide support for loading a specific file extension. --- OR better yet, we just make the equivalent of beforeLoad part of the hook api in addition to being a file in your app.)

[sgress454]

I think beforeLoad sounds great. It could have a signature like function(config, cb), where it receives the current config "overrides" (command-line args, .sailsrc options, etc.) and can programmatically decide what to do with them. It would allow you to specify hooks on a per-environment basis, which would be a big win.

Something else to throw into the mix is the idea of having sails lift just run the app's app.js file if it's available, in the same way that it now detects whether there's a local copy of Sails in the project. This would ensure that the code running in your development environment (where you typically do sails lift) is the same as in a deployment situation like Heroku (which uses node app.js), even if you put some extra goodies in your app.js. As @mikermcneil said, having a beforeLoad.js file that Sails reads first off is best, because we want it to work with sails.load() as well, but there are still cases where code is best placed in app.js (virtual host setups come to mind).

[Salakar]

Would recommend updating bootstrap.js docs to avoid confusion:

Currently says 'before sail lifts' and this is called beforeLoad, what's the difference etc.

But, this sounds to me like the main purpose of beforeLoad would be to update configs, much like the hook.configure() functionality does but for all of sails config and with a cb? If that's so can we not call it the same for consistency?

In general though, sounds like something that could be useful. 👍

[mikermcneil]

@Salakar thanks, great point re docs!

But, this sounds to me like the main purpose of beforeLoad would be to update configs, much like the hook.configure() functionality does but for all of sails config and with a cb? If that's so can we not call it the same for consistency?

Hmm, I see what you mean. This is slightly different in that it runs even before any hooks are loaded, and can even affect what languages are understood when requiring files from the config folder.

---

Here's what I'm thinking we should do for Sails v1:

• Remove all coffeescript / configurable language support from the moduleloader.
• Document how to use app.js (or app.ts or app.whatever) to support TypeScript, CoffeeScript, and Babel on the backend.
• Leave sails lift as-is for the time being. (Note that this means that, to take advantage of app.js, you'll have to start your app in development by running node app or npm start-- you won't be able to use sails lift. Let's keep brainstorming about ideas on how to improve that, but go ahead and go w/ this plan as-is for the moment.)