sandstorm-io / meteor-spk

Tool for packaging Meteor apps for Sandstorm.io
Other
29 stars 17 forks source link

Set Meteor.settings.public.sandstorm to true #22

Closed mitar closed 8 years ago

mitar commented 8 years ago

Rocket.Chat and wekan both set Meteor.settings.public.sandstorm to true and do not use the default environment variable because this allows detecting it also on the client side in Meteor. I propose this becomes the standard. It is simply to do that by changing the default environ array in sandstorm-pkgdef.capnp to include:

(key = "METEOR_SETTINGS", value = "{\"public\": {\"sandstorm\": true}}"),

I have it like so:

const myCommand :Spk.Manifest.Command = (
  # Here we define the command used to start up your server.
  argv = ["/sandstorm-http-bridge", "4000", "--", "node", "start.js"],
  environ = [
    # Note that this defines the *entire* environment seen by your app.
    (key = "PATH", value = "/usr/local/bin:/usr/bin:/bin"),
    (key = "METEOR_SETTINGS", value = "{\"public\": {\"sandstorm\": true}}"),
    # Configure Meteor settings so that the Meteor app running within Sandstorm
    # can detect if running inside Sandstorm at runtime, switching UI and/or backend
    # to use the app's Sandstorm-specific integration code.
  ]
);
mitar commented 8 years ago

cc @mquandalle

mquandalle commented 8 years ago

I’m not sure about this one. Autosetting the METEOR_SETTINGS variable seems like too much magic for me—an app author is likely to set its own sandstorm-pkgdef.capnp which will overwrite this default METEOR_SETTINGS, and that may causes some confusion. What’s more Meteor settings are supposed to be used for app-level settings, not things related to the environment (like dev/prod, or MAIL_URL, etc.), so maybe we should use a different environment variable?

The idea would be to find a variable name that is standard in NodeJS to specificity the environment target and that Meteor 1.3+ application code would be able to access without hack (like for instance the process.env.NODE_ENV).

mitar commented 8 years ago

What’s more Meteor settings are supposed to be used for app-level settings, not things related to the environment (like dev/prod, or MAIL_URL, etc.), so maybe we should use a different environment variable?

Not really. If you want to communicate something to the client, then using settings is a way.

We can also just add custom stuff to __meteor_runtime_config__ on the server, if we want. That is also send to the client. If we do not want to use Meteor settings. We could make it so that we set __meteor_runtime_config__.SANDSTORM to true if env variable is set to true.

kentonv commented 8 years ago

Seems related to: https://github.com/sandstorm-io/meteor-accounts-sandstorm/pull/20

vagrant-spk automatically initializes sandstorm-pkgdef.capnp to set the environment variable SANDSTORM=1, though this is a relatively recent change and I believe Wekan's sandstorm-pkgdef.capnp predates it.

Perhaps instead accounts-sandstorm should check for the environment variable and, if set, automatically set Meteor.settings.public.sandstorm = true in startup code? Wekan coincidentally will require no changes even though it is missing the SANDSTORM=1 env var because it just so happens to be setting Meteor.settings.public.sandstorm = true already.

mitar commented 8 years ago

accounts-sandstorm

That, or some other Meteor integration package.

But yes, maybe if having SANDSTORM environment variable is common, it is easier to use it to during runtime set Meteor.settings.public.sandstorm = true.

mitar commented 8 years ago

I think it is better to go by setting __meteor_runtime_config__.SANDSTORM to true when in Sandstorm based on the environment variable. I updated my pull request so.

In this way settings is something users can configure, while fact that runtime runs inside Sandstorm is a constant.

kentonv commented 8 years ago

BTW @mquandalle I've been trying to get a hold of you! Do you check your email? :)

mitar commented 8 years ago

This has been resolved to my satisfaction.