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

Is there interest in having app.extend? #3499

Closed
brunoscopelliti opened this issue Dec 6, 2017 · 3 comments
Closed

Is there interest in having app.extend? #3499

brunoscopelliti opened this issue Dec 6, 2017 · 3 comments

Comments

@brunoscopelliti
Copy link

Sorry if this was asked before, but I didn't find anything.

app.set always replace the whole field.

app.set("foo", "bar");
app.set("foo", "lol");
app.get("foo"); // lol

Sometimes we might want to only update certain properties.

app.set("foo", { a: 1, b: 1 });
app.set("foo", { b: 2 });
app.get("foo"); // { b: 2 }

I would like the above snippet to act like:

app.set("foo", { a: 1, b: 1 });
app.set("foo", Object.assign({}, app.get("foo"), { b: 2 }));
app.get("foo"); // { a: 1, b: 2 }

Is there any interest in having a such utility exposed such as app.extend (or whatever else), so that it would be possible to write:

app.set("foo", { a: 1, b: 1 });
app.extend("foo", { b: 2 });
app.get("foo"); // { a: 1, b: 2 }

Eventually I could submit a PR.

@dougwilson
Copy link
Contributor

Hi @brunoscopelliti currently the app settings mechanism is designed to be a single key to a single value store, and so things like app.extend are out of scope of the current design. That doesn't mean the design can't be changed, but it was a known stopping point when settings were made to not support that.

I'm going to leave this as the start of a discussion here on if this should actually be something supported.

@brunoscopelliti
Copy link
Author

What were the reasons behind that decision?
What would be the impacts of changing the design?

@dougwilson
Copy link
Contributor

At it's core, it's not really much more than app.set('foo', Object.assign(app.get('foo') || {}, { foo: 'bar' })) but without the flexibility to perform your own merge strategies, it just seems more complexity into core than it's worth... This issue is stale at this point, so just closing as stale. Don't take that as a definite "no", but more like we're not going to focus on this in the near term.

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

No branches or pull requests

2 participants