Rework framework adapters, and add new ones #61

kraftwerk28 · 2021-10-18T11:04:39Z

No description provided.

KnorpelSenf

The http and https adapters correspond to the native Node modules. They are not intended to behave differently per platform.

Thanks for adding support for the std/http server of Deno! Rather than making the existing http identifier ambiguous, we should add a new name for the adapter. Pick any you like.

Your PR starts dividing the supported platforms into the platform-specific code. This used not to be the case. I don't have strong opinions on the matter, but here are pros and cons of platform-specific code.

Pro:

no superfluous entries in autocomplete

Con:

~~missing Node adapters in API reference~~ (edit: they don't show up there)
increases dependency on platform.{deno,node}.ts

KnorpelSenf · 2021-10-26T08:06:08Z

Now that I think about it, it does indeed make a lot of sense to keep runtime-specific adapters in runtime-specific files.

Feel free to restructure platform.{deno,node}.ts and convenience/webhook.ts as you desire. It may be the case that some code needs to be duplicated in order to avoid cyclic dependencies between the two files.

It would still be preferable if there were no name clashes, i.e. that no framework adapter appears in both environments.

KnorpelSenf · 2021-11-07T13:20:02Z

@kraftwerk28 awaiting your review.

KnorpelSenf · 2021-11-15T00:06:39Z

src/convenience/frameworks.deno.ts

+ const reqUrl = new URL(req.url);
+ return {
+ update: (req.method !== "POST" || reqUrl.pathname !== secretPath)
+ ? Promise.resolve(undefined)


Why resolving undefined? I'd expect a rejection here.

package.json

src/convenience/frameworks.node.ts

tsconfig.json

KnorpelSenf · 2021-11-15T00:50:37Z

src/convenience/webhook.ts

- timeoutMilliseconds = 10_000,
-) {
- const server = frameworkAdapters[framework] ?? standard;
+ adapter?: A,


I missed this. This is a breaking change so we either have to wait with merging until 2.0 (some time later next year maybe?), or it has to be reverted.

Alternatively, we could introduce a union type. This would still permit string identifiers (non-breaking) but optionally allow people to pass their own adapter. May make it hard to find good types, not sure.

Sure, this should be in the next major. Thought I feel that giving a choice among predefined frameworks has less flexibility than allowing to put either a custom one or import from grammY's code. I'll add a temporary union

I don't see why users would have to access the predefined objects. How about not exposing them, and simply allowing string identifiers as well as custom objects? One could pass a string to use a predefined one, or an object to define a custom one.

Is there a reason for exposing them?

This way it's ideal

That's also what I have in mind. Plus, letting the adapters become implementation details that aren't exported.

KnorpelSenf · 2021-11-19T13:23:15Z

Why the force-push? Did anything else change?

kraftwerk28 · 2021-11-19T13:49:53Z

I did a pull-rebase from main branch

kraftwerk28 · 2021-11-19T13:54:18Z

Is it OK to disable no-explicit-any lint rule for frameworks.*.ts?

KnorpelSenf · 2021-11-19T13:55:47Z

I see. Now I'll need to go through all old commits again. Merge commits are usually much easier to review. Now that we're squash-merging PRs, so having a few merges on the feature branches isn't problematic.

KnorpelSenf · 2021-11-19T13:56:38Z

Is it OK to disable no-explicit-any lint rule for frameworks.*.ts?

Yes.

KnorpelSenf

IIRC we agreed on proceeding like this.

Do not support path matching for now, introduce this with a breaking change in the next major version which can perform more substantial refactoring.
Do not expose all config objects. Instead, let them be identified via strings. It is still allowed to pass your own config object, which effectively allows you to do path matching manually.
The current organisation of files (separating Deno/Node) can stay that way, as long as 2 is applied.

Make sure the identifier of the adapters do not change. This is currently the case for at least awsLambda.

KnorpelSenf · 2021-11-21T01:14:37Z

src/convenience/frameworks.node.ts

+ req.method !== "POST" ||
+ !safeStringCompare(secretPath, req.url ?? "")
+ ) {
+ reject();


Please reject with an error that has a helpful error message

KnorpelSenf · 2021-11-21T01:15:05Z

src/convenience/frameworks.node.ts

+ const raw = Buffer.concat(chunks).toString("utf-8");
+ resolve(JSON.parse(raw));
+ })
+ .once("error", (e) => reject(e));


nit:

Suggested change

.once("error", (e) => reject(e));

.once("error", reject);

KnorpelSenf · 2021-11-21T01:26:23Z

src/convenience/webhook.ts

+ try {
+ await timeoutIfNecessary(
+ bot.handleUpdate(await update, webhookReplyEnvelope),
+ typeof onTimeout === "function"
+ ? () => onTimeout(...args)
+ : onTimeout,
+ timeoutMilliseconds,
+ );
+ if (end !== undefined && !usedWebhookReply) end();
+ } catch {
+ // Adapter rejected processing an update
+ }


Also, we should not suppress errors, but throw them instead.

Does it mean an error occured if the adapter just rejected the request from Telegram (e.g. requrst.url !== secretPath)? Or we can create an class UpdateRejected extends Error and throw iif !(error instanceof UpdateRejected).

We are not going to support secret path matching in this iteration. If an adapter throws, something is wrong.

If we later implement adapters that choose not to process some requests, they must integrate with their respective web frameworks, and e.g. call next().

KnorpelSenf

LGTM now.

As stated in the conversation above, we need to change the call signature of webhookCallback in order to support more complex options objects in a usable and convenient way. This is a breaking change that will be introduced in the next major release.

Until then, users must route their updates manually, and perform necessary path matching themselves if they decide to go for a low-level framework that does not support this out of the box.

KnorpelSenf · 2021-11-29T15:54:42Z

Thank you for this contribution, and my apologies that it took so long to get this through.

@all-contributors add @kraftwerk28 for code, ideas, and review!

allcontributors · 2021-11-29T15:54:51Z

@KnorpelSenf

I've put up a pull request to add @kraftwerk28! 🎉

KnorpelSenf reviewed Oct 18, 2021

View reviewed changes

KnorpelSenf mentioned this pull request Oct 25, 2021

[Bug?] Webook reply does not work without explicitly setting Content-Type: application/json #69

Closed

kraftwerk28 force-pushed the fix-http-adapter branch from 89b2c2b to 4e6f075 Compare October 28, 2021 21:47

KnorpelSenf reviewed Nov 15, 2021

View reviewed changes

kraftwerk28 force-pushed the fix-http-adapter branch 2 times, most recently from f30722f to 680358e Compare November 19, 2021 13:18

KnorpelSenf mentioned this pull request Nov 19, 2021

Add Deno.serveHttp adapter for webhookCallback #91

Merged

KnorpelSenf requested changes Nov 21, 2021

View reviewed changes

KnorpelSenf reviewed Nov 21, 2021

View reviewed changes

kraftwerk28 force-pushed the fix-http-adapter branch from 71da33a to 80650c3 Compare November 21, 2021 17:04

kraftwerk28 and others added 12 commits November 21, 2021 19:16

Add http adapters for Deno & Node

dc852fa

Fix fmt errors

2f5a271

Fix fmt errors

a3a1b2a

Make frameworks runtime-specific

e1adfff

Update @types/node dependency

070be61

Fix compilation error

92568da

Type-check native node adapters

87a960a

Move std/http adapter to Deno-specific module

c3751f4

Align types and implementations

c3a199d

Refactor webhook adapters take grammyjs#2

9005507

Fix jsdoc

b808055

Fix: PR comments

c14c0fb

kraftwerk28 added 3 commits November 21, 2021 19:16

Run deno fmt

eed0075

Fix wrong import

4857277

Ignore no-explicit-any

79efe98

kraftwerk28 force-pushed the fix-http-adapter branch from 80650c3 to 79efe98 Compare November 21, 2021 17:16

kraftwerk28 added 3 commits November 21, 2021 21:34

Remove secretPath checking

691f916

Fix accientally removed property

e71e10b

Merge branch 'main' of github.com:grammyjs/grammY into fix-http-adapter

2c5e3bc

KnorpelSenf added the enhancement New feature or request label Nov 22, 2021

KnorpelSenf added 2 commits November 29, 2021 16:36

Revert breaking changes and minimise diff

9bef7e3

Reintroduce missing callback adapter

5549cb7

KnorpelSenf approved these changes Nov 29, 2021

View reviewed changes

Merge branch 'main' into fix-http-adapter

28f8b3b

KnorpelSenf changed the title ~~Add http adapters for Deno & Node~~ Rework framework adapters, and add new ones Nov 29, 2021

KnorpelSenf merged commit 14895d4 into grammyjs:main Nov 29, 2021

allcontributors bot mentioned this pull request Nov 29, 2021

docs: add kraftwerk28 as a contributor for code, ideas, review #114

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework framework adapters, and add new ones #61

Rework framework adapters, and add new ones #61

kraftwerk28 commented Oct 18, 2021

KnorpelSenf left a comment •

edited

Loading

KnorpelSenf commented Oct 26, 2021

KnorpelSenf commented Nov 7, 2021

KnorpelSenf Nov 15, 2021

KnorpelSenf Nov 15, 2021

KnorpelSenf Nov 15, 2021

kraftwerk28 Nov 15, 2021

KnorpelSenf Nov 15, 2021 •

edited

Loading

KnorpelSenf Nov 19, 2021

rojvv Nov 19, 2021

KnorpelSenf Nov 19, 2021

KnorpelSenf commented Nov 19, 2021

kraftwerk28 commented Nov 19, 2021

kraftwerk28 commented Nov 19, 2021 •

edited by KnorpelSenf

Loading

KnorpelSenf commented Nov 19, 2021

KnorpelSenf commented Nov 19, 2021

KnorpelSenf left a comment •

edited

Loading

KnorpelSenf Nov 21, 2021

KnorpelSenf Nov 21, 2021

KnorpelSenf Nov 21, 2021 •

edited

Loading

kraftwerk28 Nov 21, 2021

KnorpelSenf Nov 21, 2021

KnorpelSenf left a comment

KnorpelSenf commented Nov 29, 2021

allcontributors bot commented Nov 29, 2021

Rework framework adapters, and add new ones #61

Rework framework adapters, and add new ones #61

Conversation

kraftwerk28 commented Oct 18, 2021

KnorpelSenf left a comment • edited Loading

Choose a reason for hiding this comment

KnorpelSenf commented Oct 26, 2021

KnorpelSenf commented Nov 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KnorpelSenf Nov 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KnorpelSenf commented Nov 19, 2021

kraftwerk28 commented Nov 19, 2021

kraftwerk28 commented Nov 19, 2021 • edited by KnorpelSenf Loading

KnorpelSenf commented Nov 19, 2021

KnorpelSenf commented Nov 19, 2021

KnorpelSenf left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KnorpelSenf Nov 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KnorpelSenf left a comment

Choose a reason for hiding this comment

KnorpelSenf commented Nov 29, 2021

allcontributors bot commented Nov 29, 2021

KnorpelSenf left a comment •

edited

Loading

KnorpelSenf Nov 15, 2021 •

edited

Loading

kraftwerk28 commented Nov 19, 2021 •

edited by KnorpelSenf

Loading

KnorpelSenf left a comment •

edited

Loading

KnorpelSenf Nov 21, 2021 •

edited

Loading