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

Support withSchema [with PR] #433

Open
Artaud opened this issue Apr 18, 2024 · 5 comments
Open

Support withSchema [with PR] #433

Artaud opened this issue Apr 18, 2024 · 5 comments

Comments

@Artaud
Copy link
Contributor

Artaud commented Apr 18, 2024

I'd like to dynamically change schema based on request params, which can be done in Sequelize by calling the model with withSchema, similarly to withScope usage which is already supported by feathers-sequelize.

Is that something you would accept in feathers-sequelize?
I can propose a PR.

@Artaud
Copy link
Contributor Author

Artaud commented Apr 18, 2024

At the currently supported version of sequelize, it would be just schema which is supposed to be deprecated by withSchema in sequelize 7

@Artaud
Copy link
Contributor Author

Artaud commented Apr 19, 2024

I also see that you are currently getting to a new major release. I'd love to slip this addition in :)

@Artaud
Copy link
Contributor Author

Artaud commented Apr 19, 2024

I'm not sure how to structure the addition though. I see that applyScope is deprecated in favor of ModelWithScope, which is being used as a model in all DB methods.
If I wanted to add schema support, I either can:

  1. add a new function similar to applyScope, like applySchema
  2. alter the ModelWithScope to be loosened to ModelWithScopeAndSchema which I really don't like for its smell, or
  3. assume that support for more sequelize props can be added in the future, and make the Model function more generic such as:
ModelWithSequelizeParams(params: ServiceParams) {
  const Model = this.getModel(params)
  # apply scope if present in params
  # apply schema if present in params
  # here be space for future additions
  return Model
}

and then use this in all DB methods.

What do you think?

@Artaud Artaud changed the title Support withSchema Support withSchema [with PR] Apr 30, 2024
@DaddyWarbucks
Copy link
Contributor

I am sorry for the slow response on this! I will investigate Sequelize's withSchema options and see if we can get that added.

@Artaud
Copy link
Contributor Author

Artaud commented Jun 20, 2024

Thanks! I can see that you've got a lot on your hands (don't we all :D) so I completely understand, no rush :)
I've been already using the change i proposed in the PR in production (using patch-package) and it's been working good so far.

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