Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Append always a public property to Meteor.settings.public #4704

Conversation

@TomFreudenberg
Copy link
Contributor

@TomFreudenberg TomFreudenberg commented Jul 8, 2015

If no settings are given via --settings or METEOR_SETTINGS the structure is only initialized as an empty object. This causes, that the __meteor_runtime_config__.PUBLIC_SETTINGS is left empty.

In that case you can't set client settings during Meteor.startup(). In case of the missing initialized __meteor_runtime_config__.PUBLIC_SETTINGS changes are not "seen" on the client side.

Meteor.startup(function() {
  Meteor.settings.public.my_opt = "Available";
});

It would be great if this could be appended, then a developer can add even public settings e.g. during initialization of packages.

Cheers,
Tom

@TomFreudenberg
Copy link
Contributor Author

@TomFreudenberg TomFreudenberg commented Jul 14, 2015

@robertpitt

This is not the same, because later on in your app you would like to extend the Meteor.settings.public object and this should be publish automatically to clients.

If you assign (as you did) an empty new object to __meteor_runtime_config__ you had to use this identifier instead. This not a wanted behavior.

So I still please to extend the Meteor.settings object with a public element before assigning to internal __meteor_runtime_config__

Cheers, Tom.

@robertpitt
Copy link
Contributor

@robertpitt robertpitt commented Jul 14, 2015

Ahh I see now, good point.

@ErikAbele
Copy link

@ErikAbele ErikAbele commented Jul 14, 2015

+1 – just hit the same problem… :-/

@stubailo
Copy link
Contributor

@stubailo stubailo commented Jul 14, 2015

Merged: 09e07c6

@stubailo stubailo closed this Jul 14, 2015
@ErikAbele
Copy link

@ErikAbele ErikAbele commented Jul 14, 2015

@stubailo Sweet – thank you!

@TomFreudenberg
Copy link
Contributor Author

@TomFreudenberg TomFreudenberg commented Jul 15, 2015

Thanks Sashko ( @stubailo )

@dgreensp
Copy link
Contributor

@dgreensp dgreensp commented Sep 9, 2015

Is there any reason we have to set Meteor.settings.public to {} on the server? @phfrohring points out that this is noticeable to the app and seemingly contrary to the docs.

@TomFreudenberg
Copy link
Contributor Author

@TomFreudenberg TomFreudenberg commented Sep 9, 2015

Hi David ( @dgreensp )

I am sorry to ask, but I can't understand your question / annotation currently.

From my tests, if you do not initialize an empty public object during bootstrap, you can not add own elements to public send to client e.g. during startup or somewhere else in your app. This happen if you do not set a initializing structure to public via command-line or ENV-Var.

Looking forward to your comments.

Tom

@TomFreudenberg
Copy link
Contributor Author

@TomFreudenberg TomFreudenberg commented Sep 9, 2015

I read #5118 and I got your question. This is some kind of decision you have to take.

If you do not applly this patch, then you can't send settings to the client later in your app without initializing a settings structure via command-line or ENV.

IMHO I would suggest to change the doc like:

If you don't provide any settings, Meteor.settings will have only an empty public element.

Tom

@glasser
Copy link
Member

@glasser glasser commented Sep 9, 2015

I think this is a positive usability change. For example, you can have an app with optional settings, and check Meteor.settings.public.x whether or not settings are passed without having to also check if Meteor.settings.public exists.

@dgreensp
Copy link
Contributor

@dgreensp dgreensp commented Sep 9, 2015

Ok, thanks for weighing in @TomFreudenberg and @glasser. I'm inclined to leave the code as is and change the docs.

@TomFreudenberg, I did not realize that mutating Meteor.settings.public at runtime would change the settings sent to the client. It seems dangerous to depend on this behavior if it isn't documented. I added a comment about this behavior in the code, and probably we should just document it at this point.

@TomFreudenberg
Copy link
Contributor Author

@TomFreudenberg TomFreudenberg commented Sep 9, 2015

@dgreensp thanks for that decision and for updating the docs.

dgreensp added a commit that referenced this pull request Sep 9, 2015
Fix docs to say:
* Not that Meteor.settings is empty with no settings
* Meteor.settings.public is always there on client and server
* Changing it on the server affects the client

Also add mention to History.md
@dgreensp
Copy link
Contributor

@dgreensp dgreensp commented Sep 9, 2015

Docs updated in 4df1327.

@TomFreudenberg
Copy link
Contributor Author

@TomFreudenberg TomFreudenberg commented Sep 9, 2015

👍

@stubailo
Copy link
Contributor

@stubailo stubailo commented Sep 10, 2015

Awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants