search

denoland / fresh

The next-gen web framework.
https://fresh.deno.dev
MIT License
12.54k stars 648 forks source link

fix: Include submitter data in partial form data #2307

Closed adamgreg closed 5 months ago

adamgreg commented 9 months ago

Fixes #2306

adamgreg commented 9 months ago

Awesome, this is great! Thanks for opening a PR 👍 Let's add a test case and then it's ready to be merged

Hi @marvinhagemeister I've tried adding a simple test case, but it's failing and I don't understand why. I've tried the fixture pages in my browser, and they behave as I would expect, but the assertion in the test itself fails. I have a feeling the problem will be obvious to you, so please could you take a look?

adamgreg commented 8 months ago

TLDR: The feature works fine, but unit tests require a newer version of Puppeteer, which is not easily available.

Fresh uses a Deno fork of Puppeteer version 16.2.0 from deno.land/x . That installs and runs an old version of Headless Chrome: HeadlessChrome/105.0.5173.0 (mid-2022). Support for adding submitter data to formData was not added until Chrome v112: https://chromestatus.com/feature/5066604734316544. Unfortunately, 16.2.0 is the latest version available of Luca's Deno fork.

The good news is that it's now possible to run the latest version of Puppeteer directly from npm:puppeteer... The bad news about that is that it leaks ops and resources: https://github.com/puppeteer/puppeteer/issues/11839 . Tests will only pass if the sanitizers are disabled. I've tried monkey-patching NodeWebSocketTransport as suggested in that issue - it dealt with the leaked ops, but not some leaked timer resources.

An update to deno-puppeteer would solve the problem, but it seems unlikely judging by https://github.com/lucacasonato/deno-puppeteer/issues/64.

adamgreg commented 8 months ago

@marvinhagemeister as a quick fix I could add the submitter name&value manually to the formData instead of depending on the constructor to do it?

EDIT: I've added a commit to do that, which should make the unit tests pass.

adamgreg commented 8 months ago

Hi @marvinhagemeister is this OK? I'm not a fan of adding the form data manually, but it has the same effect and seems to be the only way to make it testable with this version of Puppeteer.

adamgreg commented 5 months ago

@marvinhagemeister excited to see you've switched the unit tests to Astral! Good to bring this PR back to life, and very much looking forward to Fresh 2.0! :smile: