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

Package exports for Vercel Edge Runtime support #441

Merged
merged 1 commit into from
Jun 26, 2023

Conversation

PonomareVlad
Copy link
Member

@PonomareVlad PonomareVlad commented Jun 20, 2023

Details

Vercel Edge Runtime need a different key in "exports" for web target

If we add a "browser" target, Vercel Edge Runtime can automatically switch to web build instead specifying it manually:

import { Bot } from "grammy/web"; // now

import { Bot } from "grammy"; // after merge

Also, this can solve incompatibility for some plugins that not depend on Node internal packages, but uses default import for core grammY package

Might be able to fix

How to test

I think we need to test this change on other platforms before merging 🌚

{
  "dependencies": {
    "grammy": "npm:@ponomarevlad/[email protected]"
  }
  "overrides": {
    "grammy": "npm:@ponomarevlad/[email protected]"
  }
}

Where need to test

Complete example for Vercel Edge Runtime

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.

Why isn't this covered by default? 🫠

@codecov
Copy link

codecov bot commented Jun 20, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (985e67b) 46.55% compared to head (5eeb2bb) 46.55%.

❗ Current head 5eeb2bb differs from pull request most recent head da6fa8f. Consider uploading reports for the commit da6fa8f to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #441   +/-   ##
=======================================
  Coverage   46.55%   46.55%           
=======================================
  Files          19       19           
  Lines        5595     5595           
  Branches      222      222           
=======================================
  Hits         2605     2605           
  Misses       2988     2988           
  Partials        2        2           
Impacted Files Coverage Δ
src/composer.ts 93.72% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@PonomareVlad PonomareVlad marked this pull request as ready for review June 20, 2023 15:37
@PonomareVlad
Copy link
Member Author

Why isn't this covered by default? 🫠

@balazsorban44 Can you comment on this question or suggest someone from @vercel ?

@PonomareVlad PonomareVlad force-pushed the exports-for-vercel-edge branch 2 times, most recently from 044d696 to 5ed1436 Compare June 26, 2023 21:51
src/composer.ts Outdated Show resolved Hide resolved
src/composer.ts Outdated Show resolved Hide resolved
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.

LGTM, thank you!

Also, please accept the invitation to join @grammyjs.

@KnorpelSenf KnorpelSenf merged commit ef1b947 into grammyjs:main Jun 26, 2023
@KnorpelSenf
Copy link
Member

Consider signing your commits in the future. We usually require this, but I used admin rights to merge anyway now.

@KnorpelSenf
Copy link
Member

@all-contributors add @PonomareVlad for the extensive testing and the code to bring grammY to a new platform

@allcontributors
Copy link
Contributor

@KnorpelSenf

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

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