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

Improve alterItems hook: can map to a newly created object #332

Merged
merged 2 commits into from
Jan 13, 2018
Merged

Conversation

beeplin
Copy link
Contributor

@beeplin beeplin commented Jan 13, 2018

Previously alterItems only accept a function that use item as a reference, and we have to create/mutate/delete properties. upon the original reference This makes a bunch of lodash tools unusable, since most lodash functions return a new object rather than changing the original input.

This PR allows alterItems accepting a function with a returned value. If the function returns a non-null value, the original item/items are replaced with the returned value; if the function returns nothing, just keep the previous behavior.

Example:

alterItems(item => {
  return _.omit(item, ['a', 'b', 'c'])
}

Please check the code and the tests with caution in case I make some terrible mistakes. ;)

@beeplin
Copy link
Contributor Author

beeplin commented Jan 13, 2018

I realized this is a breaking change, and maybe we should create a new hook for this purpose?

@eddyystop
Copy link
Collaborator

Is it really a breaking change? The current version ignores any return value and the docs state "Function modifies item in place."

I would prefer using if (typeof result === 'object' && object !== null).

I might have to delay updating the docs for 1 or 2 weeks as I'm focused on something else.

@beeplin
Copy link
Contributor Author

beeplin commented Jan 13, 2018

Is it really a breaking change?

Say someone has the following code:

alterItems(item => doSomethingForItem())

Before this PR it would work fine, but after the PR the item will be replaced by the returned value of doSomethingForItem, because item => doSomethingForItem() is equal to item => { return doSomethingForItem() }. Note the hidden return.

@eddyystop
Copy link
Collaborator

eddyystop commented Jan 13, 2018

doSomethingForItem would have to return something to have a problem. I could agrue that's bad code; that people get burned when playing with edge cases.

alterItems has not be out very long so the 'install base' is small. Buzzard is not yet released. V4.x.x is not yet published to npm.

I would rather not create YAH (yet another hook).

@beeplin
Copy link
Contributor Author

beeplin commented Jan 13, 2018

Agree. Me too hate YAH :))

So I will take your if (typeof result === 'object' && object !== null) and update the PR.

@beeplin
Copy link
Contributor Author

beeplin commented Jan 13, 2018

done.

@eddyystop eddyystop merged commit 4217d54 into feathersjs-ecosystem:master Jan 13, 2018
@eddyystop
Copy link
Collaborator

feathers-plus/docs#10

I'll wait to bump the semver.

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.

2 participants