refactor: consistent remove, perf selector, deps, tests#438
refactor: consistent remove, perf selector, deps, tests#438fratzinger wants to merge 2 commits intomasterfrom
Conversation
- .remove() behaves the same way as other methods, do a `._get()` with `raw: false` - selector only when necessary - update dependencies - update tests with NotFound integer id
|
I made a couple of comments. But they started getting redundant. I like the new select(data, params) {
return select(params, this.id)(data)
}
// If you want to get uber performant, then we can do the $select
// check ourselves, which would save result.length number of
// useless functions being made and then GC'ed
select(data, params) {
if (!params.query?.$select?.length) {
return data;
}
return select(params, this.id)(data)
}
// Then used as
return this.select(results, params)Then we no longer need to "setup" some All the |
| import { _ } from '@feathersjs/commons'; | ||
| import { | ||
| select as selector, | ||
| select as _selector, |
There was a problem hiding this comment.
I think we can just import this as select without having to rename it now. Adding selector as a method cleans up the code/renaming stuff.
| const select = isPresent(sequelizeOptions.attributes) ? this.selector(params) : undefined; | ||
| const result = instances.map((instance) => { | ||
| if (isPresent(sequelizeOptions.attributes)) { | ||
| if (select) { |
There was a problem hiding this comment.
If we update this.select, then this code simplifies to
if (isPresent(sequelizeOptions.attributes)) {
return this.select(instance.toJSON(), params);
}| return sequelize; | ||
| } | ||
|
|
||
| private selector (params?: ServiceParams) { |
There was a problem hiding this comment.
Not to be nit-picky about naming stuff, but I think this can be select instead of selector. Like I said above, if we don't have to rename the select import from commons, there is no longer a need to invent a new word selector.
There was a problem hiding this comment.
Even better. Let's change this to
select(data, params) {
return select(params, this.id)(data)
}| if (isPresent(sequelizeOptions.attributes)) { | ||
| const select = this.selector(params); | ||
| const result = instances.map((instance) => { | ||
| const result = select(instance.toJSON()) |
There was a problem hiding this comment.
Same here. If we update this.select, then this code simplifies to
const result = this.select(instance.toJSON(), params)|
@fratzinger This seems outdated. But there is some good info here around |
._get()withraw: false