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

WIP: Dove deno new #2828

Draft
wants to merge 149 commits into
base: dove-deno-new
Choose a base branch
from
Draft

Conversation

apollo79
Copy link

@apollo79 apollo79 commented Oct 22, 2022

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 dove branch 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.ts files 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

candicetate and others added 30 commits February 15, 2022 11:49
@netlify
Copy link

netlify bot commented Oct 22, 2022

👷 Deploy request for feathers-dove pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 9864dfc

@apollo79
Copy link
Author

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 deno fmt does. Currently I used the deno extension, but if we'll maybe check formatting via a GitHub Action in the future we should use deno fmt

* @param _params Service call parameters
* @returns The sanitized data
*/
async sanitizeData<X = Partial<D>>(data: X, _params?: P) {
Copy link
Author

Choose a reason for hiding this comment

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

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);
Copy link
Author

@apollo79 apollo79 Oct 22, 2022

Choose a reason for hiding this comment

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

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
@daffl
Copy link
Member

daffl commented Nov 5, 2022

I had a quick look and this look great, thank you for doing that. My initial questions would be:

  • Can we use the build script from @feathersjs/hooks to build the Deno packages into the packages/ folder (and then .gitignore those because we will only need them for publishing)?
  • What's the best way to run tests with Deno here?
  • Can we open this PR against the latest Dove branch. I'm assuming eventually we can remove the deno/ folder.

@apollo79
Copy link
Author

apollo79 commented Nov 6, 2022

Thanks for the feedback!
About the questions:

* Can we use the build script from `@feathersjs/hooks` to build the Deno packages into the `packages/` folder (and then `.gitignore` those because we will only need them for publishing)?

I hope so, I think, it should be possible with a few changes.
The problem isthat I am not able to run the build script in @feathersjs/hooks and neither the one I created for this PR which is quite similar.
The build script of @feathersjs/hooks throws the error

error: Uncaught (in promise) 'Module not found "file:///C:/C:/Users/<path_to_feathersjs_hooks>/main/hooks/src/index.ts".'

I am on Windows and it seems that there is a problem with the URL, that represents __dirname or path.join or something like that (notice the C:/C:/ in the file path).

What's the best way to run tests with Deno here?
To run the tests just run

deno test

in the folder of the module / package you want to test or in main to test all modules

Can we open this PR against the latest Dove branch. I'm assuming eventually we can remove the deno/ folder.

Yes, of course. Should I do this right now or later when some more packages are ported?

@daffl daffl mentioned this pull request Dec 2, 2022
@daffl daffl mentioned this pull request Dec 19, 2022
50 tasks
@apollo79 apollo79 mentioned this pull request Jan 18, 2023
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.