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

Web bundle concept #82

Closed
wants to merge 2 commits into from
Closed

Conversation

PonomareVlad
Copy link
Member

@PonomareVlad PonomareVlad commented Jul 13, 2023

Is this the right direction, or need something different ? 🌚

@codecov
Copy link

codecov bot commented Jul 13, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (3124f1f) 95.83% compared to head (723482e) 95.83%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #82   +/-   ##
=======================================
  Coverage   95.83%   95.83%           
=======================================
  Files           4        4           
  Lines         840      840           
  Branches      124      124           
=======================================
  Hits          805      805           
  Misses         33       33           
  Partials        2        2           

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

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.

In general, this is the right approach in the sense that this setup will create the correct build artefacts.

However, it has been a long standing problem that we used 30 copies of a slightly varying build setup across all repositories. Initially, the idea behind this was that each plugin could be maintained by a different person, so they should all have complete freedom over their setup. However, this turned out to be different in reality. Now, a handful of people all maintain several plugins each, and they communicate a lot so it's sort of like a small, integrated team. In such a world, it's rather annoying to have to remember and sync up changes between many repos. (Example: There were many PQs necessary to change the encoding option in TS config files.)

Therefore, ideally, we would want to extract the build config from the core repo and put it in a central place. All plugins could then rely on this shared setup, which would help us prevent the replication introduced in this PQ many times across many repos. We first need to work on wojpawlik/deno2node#40 and then create a new repo with a generic build script.

Since we still want to allow customisation of this setup in specific repos, we may want to export the build steps as functions from the common build script.

@wojpawlik
Copy link

You can already use deno2node API to write a shared build script. I'm happy to help :)

TypeScript expects tsconfig.json to be a local file, and it uses it to resolve source files.

@KnorpelSenf
Copy link
Member

We want to share the config between projects, and resolve files via script args

@wojpawlik
Copy link

wojpawlik commented Jul 16, 2023

The API doesn't even require tsconfig.json:

const ctx = new Context({ compilerOptions });
ctx.project.addSourceFilesAtPaths(...);

and then https://github.com/fromdeno/deno2node/blob/1461bd6e634ee3953545c4ce7823c4b024dc0793/src/cli.ts#L37-L46.

deno.jsonc Outdated
@@ -7,12 +7,14 @@
"dev": "deno fmt && deno lint && deno task test && deno task check",
"clean": "git clean -fX out test/cov_profile test/coverage coverage.lcov",
"coverage": "deno task clean && deno task test --coverage=./test/cov_profile && deno coverage --lcov --output=./coverage.lcov ./test/cov_profile",
"report": "genhtml ./coverage.lcov --output-directory ./test/coverage/ && echo 'Point your browser to test/coverage/index.html to see the test coverage report.'"
"report": "genhtml ./coverage.lcov --output-directory ./test/coverage/ && echo 'Point your browser to test/coverage/index.html to see the test coverage report.'",
"bundle-web": "mkdir -p out deno_cache && cd bundling && DENO_DIR=$PWD/../deno_cache deno run --unstable --quiet --allow-net --allow-read --allow-env=DENO_DIR,XDG_CACHE_HOME,HOME,DENO_AUTH_TOKENS --allow-write=../out,$PWD/../deno_cache bundle-web.ts dev ../src/mod.ts",

Choose a reason for hiding this comment

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

Consider setting cache root within grammY's script, and re-using it here to avoid duplication:

Suggested change
"bundle-web": "mkdir -p out deno_cache && cd bundling && DENO_DIR=$PWD/../deno_cache deno run --unstable --quiet --allow-net --allow-read --allow-env=DENO_DIR,XDG_CACHE_HOME,HOME,DENO_AUTH_TOKENS --allow-write=../out,$PWD/../deno_cache bundle-web.ts dev ../src/mod.ts",
"bundle-web": "mkdir -p out deno_cache && cd bundling && deno run --unstable --quiet --allow-net --allow-read=.. --allow-write=../out,../deno_cache https://raw.githubusercontent.com/grammyjs/grammY/v1.17.2/bundling/bundle-web.ts dev ../src/mod.ts",

Co-authored-by: Wojciech Pawlik <[email protected]>
@wojpawlik wojpawlik marked this pull request as draft July 17, 2023 12:53
@wojpawlik
Copy link

I don't know how to prevent bundling entire grammY in.

Isn't the Node build of conversations web compatible already?

@PonomareVlad
Copy link
Member Author

PonomareVlad commented Jul 17, 2023

Isn't the Node build of conversations web compatible already?

No, because oson depends on @deno/shim-deno package that imports node-specific fs apis

@PonomareVlad
Copy link
Member Author

@wojpawlik https://t.me/grammyjs/149613

@KnorpelSenf
Copy link
Member

We are going to:

  1. Create a Deno script that uses d2n and can build everything (npm package and web build)
  2. Unify the build and CI setup across the entire organisation with that script
  3. Pass in everything as arguments, so that the build output no longer depends on config files in the respective repo

I don't really know yet when this will happen, but once it does, it will supersede this.

@KnorpelSenf KnorpelSenf closed this Oct 8, 2023
@PonomareVlad PonomareVlad deleted the bundle-web branch October 8, 2023 10:19
@PonomareVlad
Copy link
Member Author

PonomareVlad commented Oct 8, 2023

If someone is looking for a way to solve the problem of running the conversations plugin in the Vercel Edge environment, it is:

{
  "dependencies": {
    "@grammyjs/conversations": "^1.1.2"
  },
  "overrides": {
    "o-son": "npm:@ponomarevlad/[email protected]"
  }
}

Full example

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.

3 participants