Skip to content

[API hook] loadSettings#903

Merged
JohnMcLear merged 1 commit intoether:developfrom
marcelklehr:feature/hook-loadSettings2
Aug 12, 2012
Merged

[API hook] loadSettings#903
JohnMcLear merged 1 commit intoether:developfrom
marcelklehr:feature/hook-loadSettings2

Conversation

@marcelklehr
Copy link
Copy Markdown
Contributor

Implement a loadSettings hook.

Fixes #856

@marcelklehr marcelklehr mentioned this pull request Jul 24, 2012
Comment thread src/node/server.js Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't use Object.freeze(Object.create(settings));. It makes debugging unnecessarily hard since console.log(context.settings) will always print out {} plus it does not protect the settings from being changed (since settings.__proto__ is still mutable)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I didn't know about __proto__.

What would you do? Make a for loop copying all properties (recursively), or should we just pass the original settings object?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just pass the original settings object.

@marcelklehr
Copy link
Copy Markdown
Contributor Author

OK, this should do.

Now, a question: Is there a way to obtain the settings on the client-side, yet? (This could be useful for things like #893. Maybe we can add a client-side loadSettings hook -- but obviously we don't want to send the whole settings object to the client as it contains delicate information.)

JohnMcLear added a commit that referenced this pull request Aug 12, 2012
@JohnMcLear JohnMcLear merged commit 14c874b into ether:develop Aug 12, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

suggesting a loadSettings Hook

3 participants