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

refactor: consistent remove, perf selector, deps, tests #438

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fratzinger
Copy link
Contributor

  • .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

- .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
@DaddyWarbucks
Copy link
Contributor

DaddyWarbucks commented Jun 17, 2024

I made a couple of comments. But they started getting redundant. I like the new this.select method. The main point I wanted to make is that if we update the select method, It becomes much cleaner to use.

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 select function every method. We can just use this.select(data, params) directly. And, that actually makes this.select a valuable method for the developer to use directly themselves if needed.

All the remove stuff looks good to me!

@@ -1,7 +1,7 @@
import { MethodNotAllowed, NotFound } from '@feathersjs/errors';
import { _ } from '@feathersjs/commons';
import {
select as selector,
select as _selector,
Copy link
Contributor

Choose a reason for hiding this comment

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

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 result = instances.map((instance) => {
if (isPresent(sequelizeOptions.attributes)) {
if (select) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we update this.select, then this code simplifies to

if (isPresent(sequelizeOptions.attributes)) {
  return this.select(instance.toJSON(), params);
}

@@ -222,6 +222,10 @@ export class SequelizeAdapter<
return sequelize;
}

private selector (params?: ServiceParams) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Even better. Let's change this to

select(data, params) {
  return select(params, this.id)(data)
}

@@ -306,6 +310,7 @@ export class SequelizeAdapter<
}

if (isPresent(sequelizeOptions.attributes)) {
const select = this.selector(params);
const result = instances.map((instance) => {
const result = select(instance.toJSON())
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. If we update this.select, then this code simplifies to

const result = this.select(instance.toJSON(), params)

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