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

refactor: move to deno 2.0 and refactor whole project to new approaches #235

Draft
wants to merge 51 commits into
base: main
Choose a base branch
from

Conversation

Satont
Copy link
Member

@Satont Satont commented Oct 24, 2024

Thats PR was created to rewrite existing code to new approaches.

Goals of this pr:

  • - use jsr.
    This will give us opportunity to release packages separately each other, and keep versions different.
  • - drop node-fetch in packages, because it's available in nodejs as global
  • - use deno 2.0 workspaces
  • - rewrite existed packages to use nodejs compatibale layer of deno. We should use node:* packages whereever is possible, to simplify writing and updating code without need to support both global runtime api's.
  • - add deno support for pockebase, since we doing only type import inside storage, and all interactions done via fetch calls.
  • - remove cloudflare kv adapter (see https://t.me/grammyjs/276567)

@Satont Satont changed the title refactor: move to deno 2.0 and refactor whole project to new features refactor: move to deno 2.0 and refactor whole project to new approaches Oct 24, 2024
@Satont Satont changed the title refactor: move to deno 2.0 and refactor whole project to new approaches WIP refactor: move to deno 2.0 and refactor whole project to new approaches Oct 24, 2024
@Satont Satont changed the title WIP refactor: move to deno 2.0 and refactor whole project to new approaches refactor: move to deno 2.0 and refactor whole project to new approaches Oct 24, 2024
@Satont Satont requested a review from KnorpelSenf October 25, 2024 10:02
@KnorpelSenf
Copy link
Member

I have not forgotten this but I had too many other things to do, sorry

Copy link
Member

@KnorpelSenf KnorpelSenf left a comment

Choose a reason for hiding this comment

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

There are already many breaking changes. What are your thoughts on making all storage adapters consistent? Some are just a simple function, some require a class to the instantiated, but it's not obvious when to use what. Have you considered aligning this (make everything a function or everything a class)?

Also, it seems like formatting is configured for the entire workspace but not applied correctly in each project. Am I missing something?

Comment on lines +1 to +3
{
"deno.enable": true
}
Copy link
Member

Choose a reason for hiding this comment

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

This file is no longer needed with a top-level deno.json. The extension enables itself automatically nowadays.

Comment on lines +21 to +26
"useTabs": false,
"lineWidth": 100,
"indentWidth": 2,
"semiColons": true,
"singleQuote": true,
"proseWrap": "preserve",
Copy link
Member

Choose a reason for hiding this comment

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

Some are the default values so you might want to skip them.

Suggested change
"useTabs": false,
"lineWidth": 100,
"indentWidth": 2,
"semiColons": true,
"singleQuote": true,
"proseWrap": "preserve",
"lineWidth": 100,
"singleQuote": true,
"proseWrap": "preserve",

Comment on lines 16 to 18
"prettier": "^3.2.5",
"rimraf": "^5.0.5",
"tsx": "^4.7.1",
Copy link
Member

Choose a reason for hiding this comment

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

Do you still need these?


All testing processed via github actions, because it depends on `mongodb` and `postgres` services. Maybe layter we'll add docker tests locally.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
All testing processed via github actions, because it depends on `mongodb` and `postgres` services. Maybe layter we'll add docker tests locally.
All testing processed via github actions, because it depends on `mongodb` and `postgres` services.
Maybe later we'll add docker tests locally.

@@ -15,7 +15,7 @@
"scripts": {
"test:deno": "echo \"Error: no tests found\"",
"prebuild": "rimraf dist",
"build": "deno2node tsconfig.cjs.json && deno2node tsconfig.esm.json && pnpm postbuild",
"build": "tsc -p tsconfig.cjs.json && tsc -p tsconfig.esm.json && pnpm postbuild",
Copy link
Member

Choose a reason for hiding this comment

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

I assume this is just for testing? Am I right that you would migrate to JSR and then skip the whole build process for publishing?

"exports": "./src/mod.ts",
"version": "2.4.2",
"tasks": {
"test": "deno test --allow-import --allow-write --no-check --allow-read --unstable ./__tests__/deno/deno.ts"
Copy link
Member

Choose a reason for hiding this comment

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

I think --unstable was deprecated

@@ -50,5 +44,11 @@
"middleware",
"free"
],
"gitHead": "a7758c4f957f103a14832088c6858d693c444576"
"gitHead": "a7758c4f957f103a14832088c6858d693c444576",
Copy link
Member

Choose a reason for hiding this comment

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

What does this do?

@@ -1,78 +1,71 @@
import test from 'node:test';
import test, {} from 'node:test';
Copy link
Member

Choose a reason for hiding this comment

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

what

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.

2 participants