Skip to content
This repository has been archived by the owner on Nov 9, 2024. It is now read-only.

Expose the specific engine client #5

Merged
merged 3 commits into from
Oct 2, 2024

Conversation

simoneNEMO
Copy link
Contributor

Quick solution to #3

A couple of thoughts:

  • It is a bit suboptimal that the client getter return any type. Ideally with a bit of Typescript voodoo one could make so that MagnifyManager.engine returns the type of the specific engine instead of the abstract MagnifyEngine and this should solve the problem with the getter type as well (also the client getter does not need to be part of the abstract MagnifyEngine)
  • The magnify service was not exported. Should we also change the filename from magnify.ts to main.ts as in most of other AdonisJS packages

tests/units/magnify_manager.spec.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@kerwanp
Copy link
Contributor

kerwanp commented Oct 2, 2024

Thanks a lot for the contribution!

It is a bit suboptimal that the client getter return any type. Ideally with a bit of Typescript voodoo one could make so that MagnifyManager.engine returns the type of the specific engine

I think you are right, this package deserve to be well typed. If you want to do it in this pull request, feel free to do it! If not I'm ok to merge as it is and I will work on it later.
Also, I think we could use https://japa.dev/docs/plugins/expect-type in the tests to ensure types are working correctly.

The magnify service was not exported. Should we also change the filename from magnify.ts to main.ts as in most of other AdonisJS packages

Definitely!

Thanks a lot for the typos aswell.

@simoneNEMO simoneNEMO force-pushed the feature/expose-client branch from a218b49 to 2039099 Compare October 2, 2024 13:32
@simoneNEMO
Copy link
Contributor Author

simoneNEMO commented Oct 2, 2024

Switched to a Map for the engines cache (which was actually never used before) and improved the MagnifyManager.engine return typing. Also tests have been updated accordingly.

@kerwanp
Copy link
Contributor

kerwanp commented Oct 2, 2024

All tests passed !

(I should really find a way to have the tests run properly in the CI)

image

@kerwanp kerwanp merged commit f620b0e into FriendsOfAdonis:main Oct 2, 2024
1 of 8 checks passed
@simoneNEMO simoneNEMO deleted the feature/expose-client branch October 2, 2024 20:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants