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

Feature Request - previousData #7

Open
DaddyWarbucks opened this issue Oct 25, 2023 · 2 comments
Open

Feature Request - previousData #7

DaddyWarbucks opened this issue Oct 25, 2023 · 2 comments

Comments

@DaddyWarbucks
Copy link

I love what you guys are doing here! I have often used these classes without actually integrating them into a feathers app. So I think this is a great idea.

Many of these classes do some initial lookup for their mutative methods. For example, https://github.com/wingshq/wings/blob/b991db0307e0ea08b7de29fe884b86018f9284ee/packages/knex/src/index.ts#L289
I have many instances where I am looking up the current record in order to do validation, side effects, etc.

const previousData = async (context) => {
  context.params.previousData = await service._get(context.id, context.params);
  return context;
}

Later hooks (both before and after) then use context.params.previousData to do all kinds of stuff. I have always wanted to be able to pass that param to the service method and skip its lookup.

// Knex update method
const oldData = params.previousData || await this.get(id, params)

There are some concerns when using the multi option. And there are also some concerns with whether the user passes the properly shaped previousData.

Any thoughts?

@daffl
Copy link
Member

daffl commented Oct 25, 2023

Thanks! I think this will be a good move forward because it'll hopefully also help to decouple Feathers services and database access. We could definitely clean up a few things when you don't have to concern yourself with external access.

I think the question here would be how previous data should behave across all instances. Not all databases require the previous data to e.g. do a patch (which seems to be much more commonly used than update).

@DaddyWarbucks
Copy link
Author

That makes sense. I know there have been some discussions about fast/bulk patches as well, which would affect this concept. This is just something I have always noticed in most adapters and thought this was a good place to document it.

Another solution may be to handle it in _findOrGet.

async _findOrGet(id: NullableAdapterId, params?: Params) {
  if (params.previousData) {
    return params.previousData;
  }
  return id === null ? await this.find(params) : await this.get(id, 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

No branches or pull requests

2 participants