WIP: Dove deno new #2828
WIP: Dove deno new #2828
Conversation
the packages have been brought up to date Mainly just copying the code from the `dove` branch. Instead of using events module from node_modules I used the eventemitter implementation from https://deno.land/x/events For testing I went with the functions exported by https://deno.land/std/testing/bdd.ts.
deno namespace and linting support is now only enabled in the `deno` and `main` folder. eslint doesn't lint `main` and `deno` anymore
👷 Deploy request for feathers-dove pending review.Visit the deploys page to approve it
|
|
One thing to talk about would be which formatter to use as the formatter included in the VSCode deno extension doesn't format in the same way as running |
| * @param _params Service call parameters | ||
| * @returns The sanitized data | ||
| */ | ||
| async sanitizeData<X = Partial<D>>(data: X, _params?: P) { |
There was a problem hiding this comment.
For typescript I had to make _params optional as in the _create method, sanitizeData is called with params that are maybe undefined
| return this.$get(id, { | ||
| ...params, | ||
| query, | ||
| } as P); |
There was a problem hiding this comment.
Here (not only here but in almost every method (_find, _create, _update, ...)) I had to cast the typ as P, because TypeScript complained about the following
Argument of type '{ query: Query; adapter?: Partial | undefined; paginate?: PaginationParams | undefined; provider?: string | undefined; route?: { ...; } | undefined; headers?: { ...; } | undefined; }' is not assignable to parameter of type 'P'.
'{ query: Query; adapter?: Partial | undefined; paginate?: PaginationParams | undefined; provider?: string | undefined; route?: { ...; } | undefined; headers?: { ...; } | undefined; }' is assignable to the constraint of type 'P', but 'P' could be instantiated with a different subtype of constraint 'AdapterParams<AdapterQuery, Partial>'.deno-ts(2345)
I don't know if there is a way to get around this without type casting (what would of course be very cool).
Btw we could just cast params as P, but I don't know if it is more readable:
return this.$get(id, {
...(params as P),
query,
});replace the code from https://deno.land/x/events with the old adapted code from nodejs This was also used before the updating of the packages for deno. The problem with the code from the events library was, that the exported EventEmitter is a class and assigning the prototype of this class, as happens in the feathers package for the services, doesn't work and would cause many errors. I decided to go with a .js file because a function doesn't work as constructor in typescript and assigning to the prototype of classes is (afaik) not possible without ignoring the line for typescript
there is one problem: How can we tell if we are in production mode in Deno? In Node it was done via the env variable NODE_ENV, but in deno, this, of course, doesn't exist. Using a Env variable is maybe not the best way to do it in deno, because then the `--allow-env` flag would be needed to run the code
|
I had a quick look and this look great, thank you for doing that. My initial questions would be:
|
|
Thanks for the feedback!
I hope so, I think, it should be possible with a few changes. I am on Windows and it seems that there is a problem with the
in the folder of the module / package you want to test or in
Yes, of course. Should I do this right now or later when some more packages are ported? |
Summary
Important:
This PR is not finished, it is a WIP!
This PR updates the packages ported in #2531 to the newest versions.
I used the
dovebranch and copied most things.The main things I changed are using the events library as EventEmitter and the std bdd testing library for tests
And finally, as it is somewhat of a convention in deno, I added
mod.tsfiles to every module.Other Information
I mentioned this PR on the discord yesterday, there was some conversation about how to go with deno before:
https://discord.com/channels/509848480760725514/930352418179391528/1033008949252857987