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

fix(ext/node): handle 'upgrade' responses #19412

Merged
merged 20 commits into from
Jun 13, 2023

Conversation

bartlomieju
Copy link
Member

@bartlomieju bartlomieju commented Jun 7, 2023

This commit adds support for "upgrade" events in "node:http"
"ClientRequest". Currently only "Websocket" upgrades are
handled. Thanks to this change package like "npm:puppeteer"
and "npm:discord" should work.

Closes #18913
Closes #17847

ext/fetch/lib.rs Outdated Show resolved Hide resolved
ext/fetch/lib.rs Outdated Show resolved Hide resolved
@bartlomieju
Copy link
Member Author

bartlomieju commented Jun 10, 2023

ws library is not properly closing the socket, it wait for 30s before timeout kills the program.

@bartlomieju bartlomieju marked this pull request as ready for review June 13, 2023 00:14
}

#[op]
pub async fn op_fetch_send(
state: Rc<RefCell<OpState>>,
rid: ResourceId,
into_byte_stream: bool,
Copy link
Member

Choose a reason for hiding this comment

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

not a big fan that we have this option and also the op.
Is there a considerable perf difference if we do have this option and call the two ops after each other for fetch?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably not - let's use this option for now - we will be rewriting all of that code to drop reqwest and we can clean it all up then.

ext/fetch/lib.rs Outdated Show resolved Hide resolved
ext/fetch/lib.rs Outdated Show resolved Hide resolved
state
.borrow_mut()
.resource_table
.add(FetchResponseResource {
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 we can clean this up down the line so that we always create a FetchResponseResource that can internally either be the full response, or deconstructed into a stream (an inner enum). And FetchResponseBodyResource goes away. The first read would deconstruct automatically. This is what we used to do for HTTP too - not sure if we still do that.

That way the caller would not have to choose whether they want to deconstruction or not. Depending on which ops you use later, it either happens or not.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

It's ok to land as is, but we should clean this up soon.

incoming.upgrade = null;

for (const [key, _value] of res.headers) {
if (key.toLowerCase() === "upgrade") {
Copy link
Member

Choose a reason for hiding this comment

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

I guess we don't use primordials in Node code, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct

@bartlomieju bartlomieju force-pushed the ext_node_http_upgrade branch from 44f50d2 to 6221008 Compare June 13, 2023 11:17
Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

LGTM

@bartlomieju bartlomieju merged commit 07cbec4 into denoland:main Jun 13, 2023
@bartlomieju bartlomieju deleted the ext_node_http_upgrade branch June 13, 2023 12:11
bartlomieju added a commit that referenced this pull request Jun 15, 2023
bartlomieju added a commit that referenced this pull request Jun 15, 2023
This commit adds support for "upgrade" events in "node:http"
"ClientRequest". Currently only "Websocket" upgrades are
handled. Thanks to this change package like "npm:puppeteer"
and "npm:discord" should work.

Closes #18913
Closes #17847
bartlomieju added a commit that referenced this pull request Jun 15, 2023
@jeroendee
Copy link

@bartlomieju but how to use the puppeteer product? Using their https://github.com/puppeteer/puppeteer/tree/main#readme example script, it fails downloading the chrome testing binary (= default node behaviour).

import puppeteer from 'npm:puppeteer';

(async () => {
  // Launch the browser and open a new blank page
  const browser = await puppeteer.launch();
  const page = await browser.newPage();

  // Navigate the page to a URL
  await page.goto('https://developer.chrome.com/');

  // Set screen size
  await page.setViewport({width: 1080, height: 1024});

  // Type into search box
  await page.type('.search-box__input', 'automate beyond recorder');

  // Wait and click on first result
  const searchResultSelector = '.search-box__link';
  await page.waitForSelector(searchResultSelector);
  await page.click(searchResultSelector);

  // Locate the full title with a unique string
  const textSelector = await page.waitForSelector(
    'text/Customize and automate'
  );
  const fullTitle = await textSelector?.evaluate(el => el.textContent);

  // Print the full title
  console.log('The title of this blog post is "%s".', fullTitle);

  await browser.close();
})();
error: Uncaught Error: Could not find Chrome (ver. 114.0.5735.133). This can occur if either
 1. you did not perform an installation before running the script (e.g. `npm install`) or
 2. your cache path is incorrectly configured (which is: /Users/.../.cache/puppeteer).
For (2), check out our guide on configuring puppeteer at https://pptr.dev/guides/configuration.
    at ChromeLauncher.resolveExecutablePath (file:///Users/.../Library/Caches/deno/npm/registry.npmjs.org/puppeteer-core/20.8.3/lib/esm/puppeteer/node/ProductLauncher.js:278:27)
    at ChromeLauncher.executablePath (file:///Users/.../Library/Caches/deno/npm/registry.npmjs.org/puppeteer-core/20.8.3/lib/esm/puppeteer/node/ChromeLauncher.js:174:25)
    at ChromeLauncher.computeLaunchArguments (file:///Users/.../Library/Caches/deno/npm/registry.npmjs.org/puppeteer-core/20.8.3/lib/esm/puppeteer/node/ChromeLauncher.js:91:37)
    at eventLoopTick (ext:core/01_core.js:183:11)
    at async ChromeLauncher.launch (file:///Users/.../Library/Caches/deno/npm/registry.npmjs.org/puppeteer-core/20.8.3/lib/esm/puppeteer/node/ProductLauncher.js:57:28)
    at async file:///Users/.../ACID/40-49/44/44.E/exp-puppeteer/main.ts:5:19

Also @lucacasonato example: #18913 targeting puppeteer-core is not working against the latest puppeteer version since the BrowserFetcher has been removed since Puppeteer 20.0.0 (migrating towards using the new Chrome Testing version)

Does this changeset target npm:puppeteer OR npm:puppeteer-core compatibility?

@marvinhagemeister
Copy link
Contributor

@jeroendee Something like this ensures that chrome is available before launching puppeteer:

import { DenoDir } from "https://deno.land/x/deno_cache@0.4.1/mod.ts";
import { join } from "https://deno.land/std@0.190.0/path/mod.ts";
import puppeteer, { Page, PUPPETEER_REVISIONS } from "npm:puppeteer-core";
import * as puppeteerBrowsers from "npm:@puppeteer/browsers";

async function launchPuppeteer(options: PuppeteerLaunchOptions = {}) {
  const dir = new DenoDir();
  const path = join(
    dir.root,
    "puppeteer",
    "chrome",
    PUPPETEER_REVISIONS.chrome,
  );
  const install = await puppeteerBrowsers.install({
    cacheDir: path,
    browser: puppeteerBrowsers.Browser.CHROME,
    buildId: PUPPETEER_REVISIONS.chrome,
  });

  const executablePath = puppeteerBrowsers.computeExecutablePath({
    browser: install.browser,
    buildId: install.buildId,
    cacheDir: path,
  });

  return await puppeteer.launch({
    ...options,
    executablePath,
  });
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants